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

Correct form padding and round corners in Firefox #3827

Merged
merged 5 commits into from Sep 26, 2023

Conversation

grantfitzsimmons
Copy link
Contributor

@grantfitzsimmons grantfitzsimmons commented Jul 21, 2023

Fixes #3189

Instructions to test:

  • Make sure that all areas with scrollbars display properly in Firefox and Chrome.
  • Make sure that corners on elements with scrollbars are still rounded
  • Make sure that padding is equal on both the left and right on forms

It shouldn't look like this:

image

image

Fixes #3189 by modifying `scrollbar-gutter`
@grantfitzsimmons grantfitzsimmons changed the base branch from production to v7.9-dev July 21, 2023 20:08
@grantfitzsimmons grantfitzsimmons added this to the 7.9.1 milestone Jul 21, 2023
@grantfitzsimmons grantfitzsimmons changed the title Fix Firefox display issues Correct form padding and round corners in Firefox Jul 21, 2023
Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

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

I would heavily defer the decision to say this is a good change or not to @specify/ux-testing:
It is fine as far as the code is concerned, though maybe [scrollbar-gutter:auto] might not be needed because it is the initial, default value. Need to look into this more, but it's not a bad decision to be declarative.
Maybe an approach like that taken in this StackOverflow answer may be worth considering as well

I can describe what these changes do, to give @specify/ux-testing a better idea on what to test.

scrollbar-gutter is a CSS property used to control how space is reserved for the scroll-bar.
We changed it from stable to auto.
Stable made it so that the space for the scroll-bar should have always been reserved, even if a scroll-bar is not needed.
Auto makes it so that space is only reserved for the scroll-bar when it is needed (when the contents are overflowing and we tell the browser to display a scrollbar)

See if these changes look okay on various screen widths: there should not be a drastic shifting of contents to accommodate a scroll-bar that is suddenly needed.

@melton-jason melton-jason requested a review from a team July 24, 2023 15:05
Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Screen.Recording.2023-07-24.at.10.41.33.AM.mov
Screen.Recording.2023-07-24.at.10.49.42.AM.mov

Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

scrollbar-gutter:auto is the default value, so you can just remove that class name


scrollbar-gutter was useful at one point to reduce needless UI shifts (expand/collapse something inside the container or go between records), but if rounded corners are a bigger issue then ok

@maxpatiiuk
Copy link
Member

maxpatiiuk commented Jul 25, 2023

Also, perhaps overflow:auto would be a better solution - it would make scrollbars display on top of content in some modern browsers, thus removing the UI shift issue, and removing the persistent scroll bars issue

https://developer.mozilla.org/en-US/docs/Web/CSS/overflow#:~:text=a%20scroll%20container.-,auto,drawn%20on%20top%20of%20the%20content%20instead%20of%20taking%20up%20space.,-Description

edit:
looks like it already does that:

overflow-scroll overflow-x-auto [overflow-y:overlay]

(it sets both directions to scroll, for browsers that don't support overlay, but then overwrites that)

thus removing scrollbar-gutter completely, or being explicit like @melton-jason suggested and leaving it there is fine

Base automatically changed from v7.9-dev to production September 25, 2023 18:42
@grantfitzsimmons grantfitzsimmons merged commit 7c3ee97 into production Sep 26, 2023
9 checks passed
@grantfitzsimmons grantfitzsimmons deleted the issue-3189 branch September 26, 2023 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Scroll bars appearing for no reason in app resources App resources corners are not rounded symmetrically
4 participants