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 navigation show on mobile per display type #293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

omurilo
Copy link
Contributor

@omurilo omurilo commented Oct 27, 2024

This PR fix the feature introduced on #290 on mobile devices, and add a feature to use globalDisplay for define the default behaviour of renderization to routes.

Copy link

vercel bot commented Oct 27, 2024

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

1 Skipped Deployment
Name Status Preview Updated (UTC)
zudoku-www ⬜️ Skipped (Inspect) Oct 27, 2024 5:18pm

@vercel vercel bot temporarily deployed to Preview – zudoku-www October 27, 2024 17:18 Inactive
Copy link

vercel bot commented Oct 27, 2024

@omurilo is attempting to deploy a commit to the Zuplo WWW Team on Vercel.

A member of the Team first needs to authorize it.

@mosch
Copy link
Contributor

mosch commented Oct 28, 2024

Hey @omurilo thanks for your contribution 👏 Great catch that the top-navigation item where not hidden on mobile yet.

This PR contains a lot of changes, and some of them have very far-reaching effects.
In order to get your changes in, lets move step by step.

I created this PR #297 that only contains a fix to hide the items on mobile as well.

Other things is see in this PR:

  • Introduce a globalDisplay to change the default behaviour of top navigation
  • Added missing openid auth type (great change, but 🙏 please get it in a seperate PR)
  • Hide all sidebar items if the top navigation item in hidden

Can you give us some details on the use-case so we can find an appropriate solution for want to achieve?

@mosch mosch self-requested a review October 28, 2024 09:36
@omurilo
Copy link
Contributor Author

omurilo commented Oct 28, 2024

hey @mosch, ok no problem on separate the PR.

About openid types I can made a new PR for fix this, no problem.

About globalDisplay I think it a great introduction to make possible define the default behavior for more than one item and then set a anon display in an specific route for example.

About hide all sidebar items when top navigation is hidden, I think it a default behavior for this functionality, if I set a topNavigation display for the id docs for example, I don't want the user have access to the sidebar of /docs, don't make sense it view an item it he doesn't access.

@mosch
Copy link
Contributor

mosch commented Oct 30, 2024

About hide all sidebar items when top navigation is hidden, I think it a default behavior for this functionality, if I set a topNavigation display for the id docs for example, I don't want the user have access to the sidebar of /docs, don't make sense it view an item it he doesn't access.

I understand. This option is not ment do implement permission, but only to controll visiblity. It would be the responsibility of the page to implement the permssion. So this configuration should be on the page, not on the menu.

Can you tell me about what you're trying to build – so it would be easier for us to come up with a recommended solution!

@omurilo
Copy link
Contributor Author

omurilo commented Nov 2, 2024

About hide all sidebar items when top navigation is hidden, I think it a default behavior for this functionality, if I set a topNavigation display for the id docs for example, I don't want the user have access to the sidebar of /docs, don't make sense it view an item it he doesn't access.

I understand. This option is not ment do implement permission, but only to controll visiblity. It would be the responsibility of the page to implement the permssion. So this configuration should be on the page, not on the menu.

Can you tell me about what you're trying to build – so it would be easier for us to come up with a recommended solution!

If the permission is only on the page, the menu is always visible and this isn't the acceptable behavior in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants