Put the children of AppNavigationItem outside of the main item #2572
Conversation
marcoambrosini
changed the title
quentinguidee
added
3. to review
There was a problem hiding this comment.
In principle, I like this idea (the grey hover background of the parent on the left of the child always bothered me), but the changes currently create invalid HTML.
See here:
The li tag has no ul parent (which it has to have) and one of it's siblings is an ul, which is not allowed, see https://html.spec.whatwg.org/#the-ul-element.
So please change it so that lis always have an ul parent and only li siblings.
raimund-schluessler
added
1. to develop
GretaD
force-pushed
the
children-outside-of-navigationitem
branch
from
fcbc460
to
edd9785
Compare
|
i have only rebased it so far, till i understand a bit better Raimunds perspective :) |
|
i finally managed to link my nc/vue with an app and tested this on mail with my new changes and it doesnt work :( |
I fixed one issue, but I can only look at the netlify docs at the moment, and they seem busy. @GretaD Maybe you can give it another try with mail? |
GretaD
force-pushed
the
children-outside-of-navigationitem
branch
from
43354b5
to
3e202d4
Compare
|
yes, it works, many thanks @raimund-schluessler |
|
@raimund-schluessler can you please sign off your commits? DCO is not passing |
Just squash them into yours. My changes were negligible (and I only have the webinterface at hand right now). |
yes, right, i can do that |
ChristophWurst
added
the
4. to release
ChristophWurst
removed
the
1. to develop
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
GretaD
force-pushed
the
children-outside-of-navigationitem
branch
from
3e202d4
to
3afa288
Compare
raimund-schluessler
added
2. developing
|
There is some more (CSS) stuff that needs fixing/adjustments. I will have a look and push some commits, if that is ok with you @GretaD. |
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
Signed-off-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>
raimund-schluessler
force-pushed
the
children-outside-of-navigationitem
branch
from
6d185a0
to
c85aa53
Compare
|
There seems to be an issue with the netlify docs when the branch is from a different repository. I have no idea why, but opening #2704 triggered the docs build and now the docs preview links to the build for #2704. I am hence closing this PR #2572 in favor of #2704. I hope that's fine with everyone. |
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 theclass="active"directly is only for the example? TheisActiveproperty 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.