Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix table outline frame calculations; removed conditional rendering of outline #396

Merged
merged 6 commits into from
Nov 10, 2024

Conversation

jseibert
Copy link
Contributor

@jseibert jseibert commented Nov 4, 2024

The TableExampleFactory creates tables with outline borders, but the outlines are never drawn. This fixes that.

@philprime
Copy link
Member

Hi Jeff, thanks for opening up this pull request.

I can confirm that the current example is not working as expected, and that your PR fixes it.

I do have a couple of comments, I'll add to the review

@philprime philprime changed the title fix table outline rendering fix: Fix table outline frame calculations; removed conditional rendering of outline Nov 5, 2024
@philprime philprime changed the title fix: Fix table outline frame calculations; removed conditional rendering of outline fix: fix table outline frame calculations; removed conditional rendering of outline Nov 5, 2024
@jseibert
Copy link
Contributor Author

jseibert commented Nov 7, 2024

I'm not set up for iOS dev at the moment so I can't take a look at the failing tests, but I have thoroughly tested this on macOS and fixed one final bug with the final offsets.

@philprime
Copy link
Member

Thank you, I can take over looking at the tests for iOS and fix it in the upcoming days

@philprime
Copy link
Member

So I fixed the expected test results, they did not include the table outline to begin with.

But it seems like there is something wrong in the CI, going to fix this on a different branch, I'll then update this PR

@philprime
Copy link
Member

alright looks like the checks are now passing.

Anymore changes from your side? Otherwise, lgtm

@jseibert
Copy link
Contributor Author

Amazing thank you! It's all working for me, let's merge!

@philprime philprime merged commit 5040c1e into techprimate:main Nov 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants