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

Wrong padding in AppShell when width sm is set in navbar #4269

Closed
joshuaavalon opened this issue May 18, 2023 · 5 comments
Closed

Wrong padding in AppShell when width sm is set in navbar #4269

joshuaavalon opened this issue May 18, 2023 · 5 comments

Comments

@joshuaavalon
Copy link

What package has an issue

@mantine/core

Describe the bug

When sm in width of Navbar is set, the padding is wrong in the AppShell. (See Image 1)

It only happens at some width length, it does not happen when

  • sm is not set
  • Width is wider (See Image 2) or narrower (See Image 3)

It also fine for Aside.

CodeSandbox Popup

Image 1
image

Image 2
image

Image 3
image

Code

import {
  MantineProvider,
  AppShell,
  Navbar,
  Aside,
  createStyles
} from "@mantine/core";

const useStyles = createStyles((theme) => ({
  aside: {
    padding: theme.spacing.md,
    background: "red",
    [theme.fn.smallerThan("md")]: {
      transform: "translateX(100%)"
    }
  },
  navbar: {
    padding: theme.spacing.md,
    background: "blue",
    [theme.fn.smallerThan("md")]: {
      transform: "translateX(-100%)"
    }
  }
}));

export default function App() {
  const { classes } = useStyles();
  return (
    <MantineProvider withGlobalStyles withNormalizeCSS>
      <AppShell
        padding="md"
        navbarOffsetBreakpoint="md"
        asideOffsetBreakpoint="md"
        aside={
          <Aside
            hiddenBreakpoint="md"
            className={classes.aside}
            width={{ sm: 300, md: 200, lg: 250, xl: 300 }}
          >
            aside
          </Aside>
        }
        navbar={
          <Navbar
            hiddenBreakpoint="md"
            className={classes.navbar}
            width={{ sm: 300, md: 200, lg: 250, xl: 300 }}
          >
            navbar
          </Navbar>
        }
      >
        Your app here
      </AppShell>
    </MantineProvider>
  );
}

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

6.0.11

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

https://codesandbox.io/s/determined-cache-v1izmk

Do you know how to fix the issue

No

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

No

Possible fix

No response

@neokidev
Copy link
Contributor

Hello!

I believe that by ignoring width setting for sizes at which the Navbar becomes hidden with the hiddenBreakpoint prop, we can address the issue. In this PR, I have implemented the solution following this approach.

I would appreciate it if you could review the changes and provide feedback. You can also test the implementation using the temporary Storybook setup I have prepared.

@joshuaavalon
Copy link
Author

I don't know about the implementation but if Aside does not has this problem, shouldn't Navbar follow the same approach?

@neokidev
Copy link
Contributor

@joshuaavalon

I'm sorry. You're right, I should have paid attention to the differences between the Aside and Navbar components.

After further investigation, I found that when the navbarOffsetBreakpoint and asideOffsetBreakpoint in the AppShell component have the same value, only the aside padding is correctly applied (if they have different values, the navbar padding should also be correct. Please try setting navbarOffsetBreakpoint="60em").

Below is a screenshot of the styles applied to AppShell-main when the size is sm. It seems that the aside padding is adjusted by overriding it with padding-right: 1rem.

style

Therefore, I have updated the PR to ensure that padding-left: 1rem; is correctly applied to the navbar. With this change, it is expected that both the aside and navbar will have the intended padding.

@joshuaavalon
Copy link
Author

Look good to me

@neokidev
Copy link
Contributor

@rtivital
This issue has been resolved with my PR, so I believe it can be closed. (I apologize for forgetting to link this issue when creating the PR.)

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