-
-
Notifications
You must be signed in to change notification settings - Fork 605
fix(ui): back-on-small-screen logic in settings #3328
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for elk-docs canceled.
|
✅ Deploy Preview for elk-zone ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bcef074 to
0b59eea
Compare
0b59eea to
a5b3c4f
Compare
| export const isSmallScreen = breakpoints.smallerOrEqual('sm') | ||
| export const isSmallScreen = breakpoints.smaller('sm') | ||
| export const isSmallOrMediumScreen = breakpoints.smaller('lg') | ||
| export const isMediumOrLargeScreen = breakpoints.between('sm', 'xl') | ||
| export const isExtraLargeScreen = breakpoints.smallerOrEqual('xl') | ||
| export const isExtraLargeScreen = breakpoints.greaterOrEqual('xl') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty to fix a couple breakpoint-related composables.
First, by convention in frameworks like Bootstrap, Tailwind or UnoCSS as used in this project (with a Tailwind-like preset I believe), named breakpoints are understood as the smallest width at which a condition is true, so for instance if a named breakpoint is lg and its value is 1024px, then we consider that the screen is lg (“large”) starting at 1024px and above. Which translates to the media query @media (min-width: 1024px) or @media (width >= 1024px).
When those frameworks offer pre-baked media queries like “not lg”, these are supposed to exclude the breakpoint value itself, and can be expressed as @media (width < 1024px), or some trick for non-supporting browsers like @media (max-width: 1023.99px) (because the max-width media query is inclusive of its value).
This means that when using useBreakpoints, in order to match the UnoCSS class and attributes prefixes like lg:foo, we should:
- Always use
breakpoints.greaterOrEqual, neverbreakpoints.greater. - Always use
breakpoints.smaller, neverbreakpoints.smallerOrEqual. - And I have no idea what
breakpoints.betweendoes, so I haven't checked what should be done there.
Based on that, there are two fixes highlighted above:
- export const isSmallScreen = breakpoints.smallerOrEqual('sm')
+ export const isSmallScreen = breakpoints.smaller('sm')This isSmallScreen composable is only used for some styling of the main entries in the sidebar. You can see it in action by setting the viewport width to 640px and 641px. At 640px, the icon-only sidebar buttons are misaligned because of the applied style, which was intended for smaller screens (width < 640px). Also this usage should probably be removed anyway because the sidebar is completely hidden at width < 640px, but that's a separate issue.
The other fix is for this logic issue, which should hopefully be obvious:
- export const isExtraLargeScreen = breakpoints.smallerOrEqual('xl')
+ export const isExtraLargeScreen = breakpoints.greaterOrEqual('xl')This means we go from a wrong media query of (width <= 1280px) to the correct (for the isExtraLargeScreen name) media query of (width >= 1280px).
While it's a big logical change, in practice there is no risk of regression because isExtraLargeScreen was actually unused at this point. (I haven't checked if it was ever used.) But this PR reintroduces one call site (as a refactoring of a xl:hidden styling attribute), so the logic needed fixing.
The
MainContentcomponent has abackOnSmallScreenboolean prop which is a variant of itsbackprop, and is used for settings pages.Based on the current code, its intent seems to have been to avoid showing a back button when the settings are shown in a 2 column layout, with the settings categories on the left and the content for the currently selected category on the right. This means that the current rendering at a 1024px width is wrong:
This is probably explained by the current template having this logic for rendering the back button:
Which is inconsistent logic that makes the
backandbackOnSmallScreenprops identical, and was the result of this commit that removed a conditional class coupled to thebackOnSmallScreenprop:af85a5e#diff-8047d450d250724e6d8a943121218d82f64e307a9055744a041d159325df2d68L20-R20
As a fix, this PR:
backandbackOnSmallScreenprops into oneback?: boolean | 'small-only'prop. (Because setting{ back: false, backOnSmallScreen: true }would be illogical.)v-ifand CSS classes.isSmallOrMediumScreenone, to use in that computed property logic.An alternative and simpler fix is to just live with the behavior that has shipped for 2+ years (i.e. remove the
backOnSmallScreenprop and change its call sites to usebackinstead). The UI logic for when to show this back button or not in Settings sub-pages is a bit involved and prone to breakage, so maybe just having those back buttons displayed (as in the screenshot above) is best?