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

[web] Drop heigh prop from Popup component #620

Merged
merged 3 commits into from Jun 14, 2023
Merged

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Jun 13, 2023

Problem

Sometimes a change in its content causes a modal dialog to have scroll, even when the viewport is big enough to allow the dialog to grow and avoid the scrollbars.

This happens because Agama limits dialog height via the Popup/height prop since 8b8de4c.

In order to keep the dialog visually stable, we intentionally prevent the dialog from changing size as content changes. However, turned out that actually it is worse as it is now than just giving it the freedom to adapt to the content as long as the viewport height permits it.

Solution

Drop and stop using the Popup/height prop and use only the large CSS class when is need to force it to occupy as much size as possible.

Testing

  • Tested manually

Screenshots

N/A.

Reviewers: Please, give it a shot by your self if you want to check the behavior.

For reverting behavior introduced at
8b8de4c.

Now we think that having scroll in Popups when there is room from them
to grow is worst than a dialog changing its size for adapting to the
content changes.
@coveralls
Copy link

coveralls commented Jun 13, 2023

Pull Request Test Coverage Report for Build 5257579685

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.005%) to 76.821%

Totals Coverage Status
Change from base Build 5256723874: -0.005%
Covered Lines: 5625
Relevant Lines: 7079

💛 - Coveralls

And limit the width of nodes using this CSS class too.
NOTE: not using "full-size" name instead because is already took.
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@dgdavid dgdavid merged commit 2a3e498 into master Jun 14, 2023
12 checks passed
@dgdavid dgdavid deleted the remove-popup-height-prop branch June 14, 2023 10:18
@imobachgs imobachgs mentioned this pull request Aug 2, 2023
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.

None yet

3 participants