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

Move migration sidebar entry to sidebar.yml #32332

Merged
merged 4 commits into from Jan 8, 2021
Merged

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Dec 4, 2020

TODO:

  • Fix active styling when the current .bd-links .btn[aria-expanded="true"] doesn't not cover the case, like in the Migration link

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 4, 2020
@XhmikosR XhmikosR added this to Inbox in v4.6.0 via automation Dec 4, 2020
@XhmikosR XhmikosR marked this pull request as ready for review December 4, 2020 09:59
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Dec 5, 2020
@XhmikosR XhmikosR requested a review from a team as a code owner December 6, 2020 16:26
@XhmikosR XhmikosR marked this pull request as draft December 6, 2020 16:43
@mdo
Copy link
Member

mdo commented Dec 9, 2020

Right now this breaks the sidebar text rendering (too bold) and I don't see a Migration link anywhere.

@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 9, 2020

Right now this breaks the sidebar text rendering (too bold) and I don't see a Migration link anywhere.

Too bold? It's bold when it's active only?

But yeah, the last patch is WIP. If you revert it, it will work. I'm just not sure what our plan is like I say in my OP:

Decide how to handle the case with more group without subpages

@XhmikosR XhmikosR marked this pull request as ready for review December 10, 2020 10:45
@XhmikosR
Copy link
Member Author

I still need some help with the remaining TODO, but assuming we always want the separator to show for each group entry that doesn't have sub pages, this should be better for maintenance.

@XhmikosR XhmikosR removed this from Review in v5.0.0-beta2 Dec 14, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta3 via automation Dec 14, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 14, 2020
@XhmikosR XhmikosR removed this from Inbox in v5.0.0-beta3 Dec 14, 2020
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Dec 14, 2020
@ffoodd
Copy link
Member

ffoodd commented Dec 14, 2020

@XhmikosR I simplified the active state markup-side, since the active class can be added on the link (as it's done for subnavs) without any side-effect.

Also fixed an invalid value CSS-side, flagged by my IDE.

<li{{ if $is_active_group }} class="active"{{ end }}>
<a href="/docs/{{ $.Site.Params.docs_version }}/{{ $group_slug }}/" class="d-inline-flex align-items-center rounded">
<li>
<a href="/docs/{{ $.Site.Params.docs_version }}/{{ $group_slug }}/" class="d-inline-flex align-items-center rounded{{ if $is_active_group }} active{{ end }}"{{ if $is_active_group }} aria-current="page"{{ end }}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yup

<li{{ if $is_active_group }} class="active"{{ end }}>
<a href="/docs/{{ $.Site.Params.docs_version }}/{{ $group_slug }}/" class="d-inline-flex align-items-center rounded">
<li>
<a href="/docs/{{ $.Site.Params.docs_version }}/{{ $group_slug }}/" class="d-inline-flex align-items-center rounded{{ if $is_active_group }} active{{ end }}"{{ if $is_active_group }} aria-current="page"{{ end }}>
Copy link
Member

Choose a reason for hiding this comment

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

Yup

v5.0.0-beta2 automation moved this from Review to Approved Jan 7, 2021
@XhmikosR XhmikosR merged commit 70175db into main Jan 8, 2021
v5.0.0-beta2 automation moved this from Approved to Done Jan 8, 2021
@XhmikosR XhmikosR deleted the main-xmr-docs-mv-migration branch January 8, 2021 07:49
@XhmikosR XhmikosR moved this from Inbox to Needs manual backport in v4.6.0 Jan 8, 2021
@XhmikosR XhmikosR removed this from Needs manual backport in v4.6.0 Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants