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

Put the children of AppNavigationItem outside of the main item #2704

Merged
merged 5 commits into from
May 25, 2022

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented May 20, 2022

Originally created by @quentinguidee in #2572:

This change is necessary for #2571 (see why in action: #2571 (comment)). It puts AppNavigationItem's children out of the parent item, and displays them separately. This is the simplest implementation I found without recoding the entire AppNavigationItem. I'm open to ideas if you find something cleaner.

before.mov
after.mov

Important notes/questions to reviewers

  • In the current implementation, a side effect is that it disables the ability to pass (like in the doc) class="active" directly to the AppNavigationItem. It seems like the only purpose of passing the class="active" directly is only for the example? The isActive property works fine and continues to push the blue background.

  • As another side effect of this PR, when hovering a child, it doesn't display the hover background of the parent (as demonstrated in the screenshots). I can go back to the first one easily if needed. Just unsure if it was a bug or not.

quentinguidee and others added 3 commits May 20, 2022 15:23
In the current implementation, a side effect is that it disables the ability to pass
class="active" directly to the AppNavigationItem.

Signed-off-by: Quentin Guidée <quentin.guidee@gmail.com>

Update src/components/AppNavigationItem/AppNavigationItem.vue

Update src/components/AppNavigationItem/AppNavigationItem.vue
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@raimund-schluessler
Copy link
Contributor Author

@GretaD I fixed some more issues (collapsing didn't work, children were not styled correctly, pinning was broken, see my commits). I would suggest you test it again with Mail, and ideally with Calendar as well, probably.
Some Apps will need adjustments of their CSS, see for example the necessary changes for Tasks in nextcloud/tasks#2007
So this will be a breaking change if we interpret it strictly.

@raimund-schluessler raimund-schluessler marked this pull request as ready for review May 20, 2022 20:10
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
@GretaD
Copy link
Contributor

GretaD commented May 23, 2022

tested with mail and looks good, but i still have this on my code

navelement

@raimund-schluessler
Copy link
Contributor Author

tested with mail and looks good, but i still have this on my code

navelement

Hm, no idea. Maybe this depends on the browser or how the app is compiled 🤔 When I use it in Tasks, it just creates a div as one would expect.

Copy link
Contributor

@GretaD GretaD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let move on then, the component works fine on mail(i tested it) and on tasks(Raimund tested it)

@ChristophWurst ChristophWurst added the breaking PR that requires a new major version label May 25, 2022
Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GretaD GretaD merged commit f8b6231 into master May 25, 2022
@GretaD GretaD deleted the pr/quentinguidee/2572 branch May 25, 2022 13:28
@raimund-schluessler raimund-schluessler added this to the 6.0.0 milestone May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews breaking PR that requires a new major version enhancement New feature or request feature: app-navigation Related to the app-navigation component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants