-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Resolve relative size rendering error in inspector #23804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues. Runyarn lint --fix
to automatically fix problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues. Runyarn lint --fix
to automatically fix problems.
@gandreadis great work 👌 if you want automatically close addressed issues which will be fixed by your PR use any magical keyword https://help.github.com/en/articles/closing-issues-using-keywords in your description. |
@radeno Thanks for the suggestion! I was a bit hesitant to do this, since I know that Facebook procedure is generally to close PRs and then to merge them manually (via a bot), so I was not sure if the GitHub keyword would still work. But I guess we'll see :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change!
Given that this new module is only used in a single file and it is relatively small, could you just inline the code in ElementBox?
Also, please fix up the flow types :)
Hi @cpojer, thanks for your suggestions! I've tried to implement them, could you have another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, that looks good to me :) Thanks so much for making this change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @gandreadis in 972ee2e. When will my fix make it into a release? | Upcoming Releases |
Summary: Currently, when relative sizes are given in margin or padding stylings (be it a percentage or an auto measure), the inspector crashes, due to frame rendering not properly handling those kinds of measurements. This PR adds a resolution step for them: * Percentages are evaluated relative to the window size. * I decided to simply not render `auto` margins/paddings, due to the complexities involved (e.g. when the margin is between multiple elements with relative sizes). Since the inspector does not crash anymore on relative sizes on paddings or margins, I believe that this addresses #17496. Fixes #17496 [General] [Fixed] - Fix inspector rendering of relative margins and paddings Pull Request resolved: #23804 Differential Revision: D14437273 Pulled By: cpojer fbshipit-source-id: c9f0f71a2e1b2399a2b2148cef2124787703ead3
Summary
Currently, when relative sizes are given in margin or padding stylings (be it a percentage or an auto measure), the inspector crashes, due to frame rendering not properly handling those kinds of measurements. This PR adds a resolution step for them:
auto
margins/paddings, due to the complexities involved (e.g. when the margin is between multiple elements with relative sizes).Since the inspector does not crash anymore on relative sizes on paddings or margins, I believe that this addresses #17496.
Fixes #17496
Changelog
[General] [Fixed] - Fix inspector rendering of relative margins and paddings
Test Plan
I ran the RNTester app with my adapted version of the Inspector. Before running it, I modified the styling of example components in different ways. I tried the following combinations:
paddingTop: 5%
paddingRight: 5%
paddingBottom: 5%
paddingLeft: 5%
marginTop: 5%
marginRight: 5%
marginBottom: 5%
marginLeft: 5%
padding: 5%
margin: 5%
marginLeft: auto
paddingLeft: auto
They can be seen in that order in the screenshots below: