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

render: fix sql table overflow #458

Merged
merged 9 commits into from
Dec 20, 2022

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Dec 18, 2022

Summary

fixes text overflowing in sql tables when a column with a short name and a long type is present in a table with a column having a long name and short type.

Details

  • fixes render: sql table text overflows here #457
  • adds sql_table_overflow e2e test for the text overflow
  • fixes width calculation for sql_table shapes in d2graph SetDimensions
  • exports SQLColumn Name and Type dimensions for rendering

e2e report changes

_Users_gavinnishizawa_github_repos_d2_out_e2e_report html (3)

@gavin-ts gavin-ts marked this pull request as ready for review December 18, 2022 03:11
@gavin-ts gavin-ts requested a review from a team December 18, 2022 03:11
@gavin-ts gavin-ts enabled auto-merge December 18, 2022 03:11
@gavin-ts gavin-ts disabled auto-merge December 18, 2022 04:48
Copy link
Contributor

@ejulio-ts ejulio-ts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at sql_table_overflow, it looks like tables are growing in size while rendering.
If that's true, could you try it with some larger diagrams so that we see if this size difference could cause issues?
Maybe, it's worth updating the table dimensions to match it properly too

@gavin-ts
Copy link
Contributor Author

looking at sql_table_overflow, it looks like tables are growing in size while rendering. If that's true, could you try it with some larger diagrams so that we see if this size difference could cause issues? Maybe, it's worth updating the table dimensions to match it properly too

What do you mean growing in size while rendering? Just that it overflowed? That seems to miss the point, the issue was that we were not measuring the table sizes correctly. we needed to measure the longest width name and the longest width type to get the width of the whole table since we align the types at the end of the longest name and this fixes that. The bug is that if you have a short name (10) and a long type (100), its width on its own is only 110+padding but if you have another where it has a long name (100) and a short type (10), it also has a width of 110+padding but combined in the same table these have a width of 200+padding since we need to align the types at the end of the names

@alixander
Copy link
Collaborator

alixander commented Dec 20, 2022

@gavin-ts simply put, layout engine will see the wrong dimensions right? that's bad.

needing the ruler during the drawing phase is also violating expectations. everything's already been exported. the renderer is like a photo printer, no more reshoots.

@gavin-ts
Copy link
Contributor Author

@gavin-ts simply put, layout engine will see the wrong dimensions right? that's bad.

needing the ruler during the drawing phase is also violating expectations. everything's already been exported. the renderer is like a photo printer, no more reshoots.

the layout engine should have the correct dimensions with this change

I can export the longest name width or offset of the type fields if we don't want to pass in the ruler

@gavin-ts gavin-ts force-pushed the fix_sql_table_overflow branch from 2980e41 to c054538 Compare December 20, 2022 01:57
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@gavin-ts gavin-ts requested a review from alixander December 20, 2022 05:04
@gavin-ts gavin-ts merged commit fa07ee5 into terrastruct:master Dec 20, 2022
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.

render: sql table text overflows here
3 participants