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

Use collapse component in docs' sidenav #30230

Merged
merged 6 commits into from
Mar 28, 2020
Merged

Use collapse component in docs' sidenav #30230

merged 6 commits into from
Mar 28, 2020

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Feb 18, 2020

@XhmikosR
Copy link
Member

XhmikosR commented Feb 18, 2020

This has the issue I also faced back then, the show/collapse is jaggy, or whatever the proper word is to describe this :P

My previous PR #29104

@XhmikosR XhmikosR added the docs label Feb 18, 2020
@ffoodd ffoodd requested a review from a team as a code owner February 18, 2020 12:49
@ffoodd
Copy link
Member Author

ffoodd commented Feb 18, 2020

@XhmikosR The struggling transitions was due to flex-column and wrapping that didn't happen at the right time. I guess it's good now.

I chose to make it in docs.css instead of using utilities since it's pretty specific: should I change it to use utilities?

@XhmikosR
Copy link
Member

XhmikosR commented Feb 18, 2020

It's fine, thanks! But do we need the custom JS code to toggle the active class now? Can't we make use of another built-in class for this here and drop the custom JS toggling code?

I remember @mdo not liking the transition there, which was another reason I dropped my PR.

@XhmikosR XhmikosR added the v5 label Feb 18, 2020
@ffoodd ffoodd requested a review from a team as a code owner February 18, 2020 13:05
@ffoodd
Copy link
Member Author

ffoodd commented Feb 18, 2020

You were right, I was able to drop the custom JS (and the associated class in markup) by simply using aria attributes as hooks for styles. It makes sense, to me.

About transition, it's all about the collapse component: I think it's more consistent to keep it, but we should be able to reset it if you decide otherwise.

@mdo
Copy link
Member

mdo commented Feb 18, 2020

You know what, I can dig it.

Makes me think that we could also, in another PR, do a docs or dashboard example that uses this same kind of layout and sidebar. We always get folks asking to use pieces of our docs :).

@ffoodd
Copy link
Member Author

ffoodd commented Feb 28, 2020

Hi there, just to let you know: my second child is just born, so don't expect me to work on this until mid-march 👶

@MartijnCuppens
Copy link
Member

Congrats @ffoodd!

@ffoodd
Copy link
Member Author

ffoodd commented Mar 12, 2020

Here I am :)

From my view, the only remaining question is whether to keep [aria-expanded="true"] selector or to switch to &.has-children .bd-sidenav-group-link:not(.collapsed) (and adding .collapsed class in markup for first load).

@MartijnCuppens what's your opinion? We may also try to use another class name beside .bd-sidenav-group-link but I'm not really sure it would work…

@MartijnCuppens
Copy link
Member

@ffoodd, in general classes are used for theming, aria labels for accesibility.

I just simplified the styles a little bit. Not sure if we should use a class name for the links. I think we can use the less strict approach here since all links have a lot of styles in common.

@ffoodd
Copy link
Member Author

ffoodd commented Mar 17, 2020

@MartijnCuppens :not(:only-child) is a nice move!

@ffoodd ffoodd self-assigned this Mar 17, 2020
@twbs twbs deleted a comment from ffoodd Mar 18, 2020
@XhmikosR XhmikosR added this to Inbox in v5 via automation Mar 18, 2020
v5 automation moved this from Inbox to Approved Mar 18, 2020
@XhmikosR
Copy link
Member

XhmikosR commented Mar 18, 2020

I like it! Less code, better for me :)

Waiting for @patrickhlauke's, @mdo's and @MartijnCuppens's approval.

EDIT: the transition is something we should vote, I can't tell I like it or not, especially for long submenus

@XhmikosR
Copy link
Member

@ffoodd can you rebase this please?

@ffoodd
Copy link
Member Author

ffoodd commented Mar 23, 2020

@XhmikosR Done :)

@XhmikosR
Copy link
Member

I'm going to merge this and we can tweak it further later.

@XhmikosR XhmikosR merged commit 4448856 into twbs:master Mar 28, 2020
v5 automation moved this from Approved to Shipped Mar 28, 2020
@ffoodd ffoodd deleted the docs/sidenav-collapse branch March 30, 2020 08:03
@inwardmovement
Copy link
Contributor

inwardmovement commented Jun 17, 2020

Did you guys consider using the accordion alternative?

At first sight I think it may be more practical and pleasant to use (+ resulting in a less long sidebar ; is it useful to have more than one section expanded at a time?)

@XhmikosR
Copy link
Member

@inwardmovement please make new issues, commenting on old stuff is hard to track.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

5 participants