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

AppShell padding error #3678

Closed
Pivigor opened this issue Mar 4, 2023 · 2 comments
Closed

AppShell padding error #3678

Pivigor opened this issue Mar 4, 2023 · 2 comments

Comments

@Pivigor
Copy link

Pivigor commented Mar 4, 2023

What package has an issue

@mantine/core

Describe the bug

AppShell uses CSS variables for padding definition like this:

paddingTop: `calc(var(--mantine-header-height, 0) + ${padding})`,
paddingBottom: `calc(var(--mantine-footer-height, 0) + ${padding})`,
paddingLeft: `calc(var(--mantine-navbar-width, 0) + ${padding})`,
paddingRight: `calc(var(--mantine-aside-width, 0) + ${padding})`,

If one of these variables is not declared, for example --mantine-header-height, then we get:

paddingTop: `calc(0 + 1rem)`,

There will be an error here, and this padding property will be skipped.

I think this is a problem because padding will not work if there is no corresponding component.
For example, if a component was temporarily hidden, this leads to unexpected jumps.

paddingTop: `calc(var(--mantine-header-height, 0) + ${padding})`,
paddingBottom: `calc(var(--mantine-footer-height, 0) + ${padding})`,
paddingLeft: `calc(var(--mantine-navbar-width, 0) + ${padding})`,
paddingRight: `calc(var(--mantine-aside-width, 0) + ${padding})`,

What version of @mantine/hooks page do you have in package.json?

6.0.0

If possible, please include a link to a codesandbox with the reproduced problem

No response

Do you know how to fix the issue

Yes

Are you willing to participate in fixing this issue and create a pull request with the fix

No

Possible fix

It seems to me that it should be like this:

paddingTop: `calc(var(--mantine-header-height, 0rem) + ${padding})`,
paddingBottom: `calc(var(--mantine-footer-height, 0rem) + ${padding})`,
paddingLeft: `calc(var(--mantine-navbar-width, 0rem) + ${padding})`,
paddingRight: `calc(var(--mantine-aside-width, 0rem) + ${padding})`,
@samzmann
Copy link

samzmann commented Mar 4, 2023

AppShell content has no paddingRight after upgrading to v6. I guess it's related to this issue.

@rtivital
Copy link
Member

rtivital commented Mar 8, 2023

Fixed in 6.0.1

@rtivital rtivital closed this as completed Mar 8, 2023
mjoblin added a commit to mjoblin/vibinui that referenced this issue Aug 29, 2023
The scrollbar appeared due to a fix to Mantine in 6.0.1:

mantinedev/mantine#3678

It turns out Vibin was relying on the bottom padding not being set
properly. This fix hardcodes the `paddingBottom` to `0` to restore the
old behavior.

This only affected the <CurrentTrack> screen.

Ideally the top-level application layout would be rethought to be less
convoluted and more clear in how it manages window height, scrollbars,
etc.
mjoblin added a commit to mjoblin/vibinui that referenced this issue Aug 29, 2023
The scrollbar appeared due to a fix to Mantine in 6.0.1:

mantinedev/mantine#3678

It turns out Vibin was relying on the bottom padding not being set
properly. This fix hardcodes the `paddingBottom` to `0` to restore the
old behavior.

This only affected the `<CurrentTrack>` screen.

Ideally the top-level application layout would be rethought to be less
convoluted and more clear in how it manages window height, scrollbars,
etc.
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

No branches or pull requests

3 participants