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

Resolve relative size rendering error in inspector #23804

Closed

Conversation

gandreadis
Copy link

@gandreadis gandreadis commented Mar 7, 2019

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

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:

screenshots

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2019
Copy link

@analysis-bot analysis-bot left a 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. Run yarn lint --fix to automatically fix problems.

Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a 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. Run yarn lint --fix to automatically fix problems.

Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
Libraries/Inspector/resolveRelativeSizes.js Outdated Show resolved Hide resolved
@radeno
Copy link
Contributor

radeno commented Mar 7, 2019

@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.
Handy feature.

@gandreadis
Copy link
Author

@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 :)

Copy link
Contributor

@cpojer cpojer left a 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 :)

@gandreadis
Copy link
Author

Hi @cpojer, thanks for your suggestions! I've tried to implement them, could you have another look?

Copy link
Contributor

@cpojer cpojer left a 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!

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 13, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gandreadis in 972ee2e.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Mar 13, 2019
@gandreadis gandreadis deleted the inspector-relative-sizing branch March 13, 2019 09:10
@react-native-bot react-native-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 13, 2019
grabbou pushed a commit that referenced this pull request May 6, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants