Skip to content

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

Merged
atmelmicro merged 1 commit into
masterfrom
bug/popover-line
Sep 13, 2024
Merged

Fix rendering glitch of Popover arrow on low-dpi displays#559
atmelmicro merged 1 commit into
masterfrom
bug/popover-line

Conversation

@atmelmicro

Copy link
Copy Markdown
Contributor

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

@adamkudrna adamkudrna left a comment

Copy link
Copy Markdown
Member

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).

Comment thread src/components/Popover/Popover.module.scss Outdated
Comment thread src/components/Popover/_theme.scss Outdated
@adamkudrna

Copy link
Copy Markdown
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

@adamkudrna adamkudrna left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/components/Popover/Popover.module.scss Outdated
Comment thread src/components/Popover/_theme.scss Outdated
Comment thread src/components/Popover/_theme.scss Outdated
@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
Comment thread src/components/Popover/_theme.scss Outdated
Comment thread src/components/Popover/Popover.module.scss Outdated

@adamkudrna adamkudrna left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor 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

mbohal commented Sep 13, 2024

Copy link
Copy Markdown
Contributor

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