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

Fix rendering glitch of Popover arrow on low-dpi displays #559

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

atmelmicro
Copy link
Collaborator

Because of rendering quirks sometimes a line can appear between the popover and arrow
Snímek obrazovky 2024-08-23 v 9 08 11
(more visible on low dpi displays)

This pr fixes that by moving the arrow 1px down
Snímek obrazovky 2024-08-23 v 9 44 18

@atmelmicro atmelmicro requested a review from adamkudrna as a code owner August 23, 2024 07:57
@github-actions github-actions bot added the fix Fixing a bug label Aug 23, 2024
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! I'd like to ask for a few minor adjustments to improve code readability and keep the existing flexibility (rem units).

src/components/Popover/Popover.module.scss Outdated Show resolved Hide resolved
src/components/Popover/_theme.scss Outdated Show resolved Hide resolved
@adamkudrna
Copy link
Member

The PR name could be like "Fix rendering glitch of Popover arrow on low-dpi displays". There is no intentional line so it cannot be removed… 🙂

@atmelmicro atmelmicro changed the title Remove line between popover and arrow Fix rendering glitch of Popover arrow on low-dpi displays Aug 26, 2024
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

OK, this took me some time to figure out, but I think I found the right calculations for what we need. You can test it with various values, for example 2 px gap and 8 px overlap or whatsoever.

src/components/Popover/Popover.module.scss Outdated Show resolved Hide resolved
src/components/Popover/_theme.scss Outdated Show resolved Hide resolved
src/components/Popover/_theme.scss Outdated Show resolved Hide resolved
@adamkudrna adamkudrna changed the title Fix rendering glitch of Popover arrow on low-dpi displays Fix rendering glitch of Popover arrow on low-dpi displays Aug 31, 2024
@adamkudrna adamkudrna added this to the v1.0.0 milestone Sep 2, 2024
src/components/Popover/_theme.scss Outdated Show resolved Hide resolved
src/components/Popover/Popover.module.scss Outdated Show resolved Hide resolved
Copy link
Member

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

It works! 🎉

@atmelmicro atmelmicro force-pushed the bug/popover-line branch 2 times, most recently from 33a9195 to ed04de4 Compare September 12, 2024 12:54
@atmelmicro
Copy link
Collaborator Author

Snímek obrazovky 2024-09-12 v 14 58 05 Hey, the build failing because of the bundle being too big. Is it safe to merge?

@mbohal
Copy link
Contributor

mbohal commented Sep 13, 2024

@atmelmicro

Please do increase the allowed size:
image

Here, as well as in our other codebases, we do not have a strict size increase process. We use the limit as a general guideline to prevent the size from exploding beyond reason as a result of bad development decision or a an error.

So if you, as the change author, consider the size increase justified, then feel free to increase the limit. The limit change will be reviewed during the CR same as any other change.

The size increase in this PR is minimal and is fine by me.

@atmelmicro atmelmicro merged commit d532592 into master Sep 13, 2024
11 checks passed
@atmelmicro atmelmicro deleted the bug/popover-line branch September 13, 2024 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fixing a bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants