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 a couple regressions found on V6 + Improve Slider hover UX #3695

Merged
merged 5 commits into from
Mar 8, 2023

Conversation

jvdsande
Copy link
Contributor

@jvdsande jvdsande commented Mar 5, 2023

Hi :)

As discussed on Discord, here are some quick fixes for some regressions I found while updating my app from v5 to v6. I

I implemented each of them in individual commits; if you prefer I split those in individual PRs, I can too.

Here is a list of what's fixed:

  • Arrow function positioning was using the raw pixel value, while the size of the arrow was using the new rem() helper. So arrow was not positioned correctly if the rem value was anything other than 16. Fixed by using rem() on positioning too.
  • Still arrow function : removed the extra offset when there is a border, see comment below. I think a display might have changed from content box to border box somewhere.
  • fn.rgba is used in more places, preventing CSS variables to be passed to some color props, instead it used a default black value. Added a check for CSS variables in fn.rgba, fn.lighten and fn.darken to prevent that.
  • Added forwardRef to InlineInput so that it works like InputWrapper

The last commit is not a regression, it was something I already wanted to fix in v5:

  • In Slider, when showLabelOnHover is true, it only works when hovering the track, not the thumb. Since the thumb is larger than the track, I added a listener on the thumb too, otherwise it was a bit awkward.

@@ -90,7 +88,7 @@ export function getArrowPositionStyles({
[radiusByFloatingSide[side]]: rem(arrowRadius),
};

const arrowPlacement = withBorder ? -arrowSize / 2 - 1 : -arrowSize / 2;
const arrowPlacement = rem(-arrowSize / 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While playing with big REM units on the Storybook to check if the arrow was positioned correctly, I found that the -1 px offset was in fact unnecessary, even at base REM value of 16px. It added an imperceptible gap that became more and more visible with bigger REM values, already quite visible at REM = 20px.

So I removed it, and consequently removed the withBorder prop altogether since it did nothing else than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here at REM = 32px:
image

Behavior if we keep the -1 offset:
image

@rtivital rtivital merged commit 8c73f33 into mantinedev:master Mar 8, 2023
@rtivital
Copy link
Member

rtivital commented Mar 8, 2023

Thanks!

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

2 participants