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: use aside.exclude for mobile aside #710

Merged
merged 1 commit into from
Dec 15, 2022

Conversation

itsmnthn
Copy link
Contributor

@itsmnthn itsmnthn commented Dec 3, 2022

components/app/AppHeaderDialog.vue component uses header.exclude to filter the navigation to display aside on mobile devices, which is actually aside but filtering with header.exclude causes filtering aside links

@vercel
Copy link

vercel bot commented Dec 3, 2022

@itsmnthn is attempting to deploy a commit to the Nuxt Labs Team on Vercel.

A member of the Team first needs to authorize it.

@itsmnthn
Copy link
Contributor Author

itsmnthn commented Dec 3, 2022

This may lead to displaying all the nav links in the aside if not excluded and excluding causes it to be excluded from desktop and mobile aside too hence I don't think this PR makes sense in this way but using header.exclude doesn't make sense to me either. I guess the better way to do this is to have two mobile nav 1. top level and 2. sub-level and swap between based on route or something

@vercel
Copy link

vercel bot commented Dec 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
docus ✅ Ready (Inspect) Visit Preview Dec 5, 2022 at 0:48AM (UTC)

@Tahul
Copy link
Contributor

Tahul commented Dec 7, 2022

@atinux ; I'm afraid merging this PR could break a lot of existing docs.

As little as this change is, I don't properly know how many websites relies on the current implementation for it to be a breaking change.

@Tahul Tahul merged commit f8e1d01 into nuxt-themes:dev Dec 15, 2022
@itsmnthn itsmnthn deleted the fix/aside-exclude branch December 15, 2022 02:55
@BracketJohn
Copy link

Heya 👋

Just as a heads-up: This actually was a (surprising) breaking change for us: sidebase/docs#59

If someone else finds this: Downgrading to at least 1.1.3 fixes the issue.

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.

3 participants