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.

Sign up for GitHub

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

Jump to bottom

Doc tweaks #29029

Merged
merged 9 commits into from
Jul 17, 2019
Merged

Doc tweaks #29029

merged 9 commits into from
Jul 17, 2019

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Jul 13, 2019 •
edited
Loading

@mdo: not sure why you used 600px, but this is the proper way to do it. If you want it bigger, we'd need a bigger set of the 2 images.

@XhmikosR XhmikosR added the docs label Jul 13, 2019
@XhmikosR XhmikosR requested a review from mdo July 13, 2019 11:29
@XhmikosR XhmikosR changed the title Use the responsive themes image. Doc tweaks Jul 13, 2019
@XhmikosR XhmikosR requested a review from a team as a code owner July 15, 2019 14:34
@XhmikosR XhmikosR force-pushed the master-xmr-docs branch 3 times, most recently from 0a65ca8 to 60e1707 Compare July 15, 2019 15:00
@XhmikosR XhmikosR mentioned this pull request Jul 15, 2019
Closed
12 tasks
@mdo mdo mentioned this pull request Jul 15, 2019
Closed
17 tasks
@XhmikosR
Copy link
Member Author

XhmikosR commented Jul 16, 2019 •
edited
Loading

IE should be fixed.

/CC @mdo for approval

Perhaps we should move the arrow in the CSS pseudo selector since it's repeated a lot.

@XhmikosR
Copy link
Member Author

XhmikosR commented Jul 16, 2019 •
edited
Loading

@patrickhlauke: should I add aria-hidden="true" in the SVG? And focusable="false"?

@XhmikosR XhmikosR force-pushed the master-xmr-docs branch 2 times, most recently from 493bfcf to b73b1b8 Compare July 16, 2019 10:40
@patrickhlauke
Copy link
Member

@patrickhlauke: should I add aria-hidden="true" in the SVG? And focusable="false"?

yes please

@mdo
Copy link
Member

mdo commented Jul 16, 2019

We can skip those SVG changes if we instead make it part of the CSS. Lemme have a crack at that.

@XhmikosR XhmikosR requested a review from a team as a code owner July 16, 2019 23:01
@mdo
Copy link
Member

mdo commented Jul 16, 2019

Pushed up the changes for the sidebar, but it occurs to me that I'm also using the same icon elsewhere in the subnav, so we likely need those optimizations as well.

@XhmikosR
Copy link
Member Author

XhmikosR commented Jul 17, 2019 •
edited
Loading

Pushed up the changes for the sidebar, but it occurs to me that I'm also using the same icon elsewhere in the subnav, so we likely need those optimizations as well.

Which optimizations are you referring to? And where is this arrow SVG being used apart from here?

XhmikosR
XhmikosR commented Jul 17, 2019
View reviewed changes
site/layouts/partials/docs-sidebar.html
@@ -17,7 +17,7 @@

<li class="bd-sidenav-group my-1{{ if $active_group }} active{{ end }} js-sidenav-group">
<a class="d-inline-flex align-items-center bd-sidenav-group-link" href="/docs/{{ $.Site.Params.docs_version }}/{{ $group_slug }}/{{ if $group.pages }}{{ $link_slug }}/{{ end }}">
<svg class="bd-sidenav-group-link-icon" viewbox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><path stroke="currentColor" stroke-width="2" d="M5 14l6-6-6-6" fill="none" fill-rule="evenodd" stroke-linecap="round" stroke-linejoin="round"/></svg>
<svg class="bd-sidenav-group-link-icon" width="16" height="16" viewbox="0 0 16 16" xmlns="http://www.w3.org/2000/svg"><path stroke="currentColor" stroke-width="2" d="M5 14l6-6-6-6" fill="none" fill-rule="evenodd" stroke-linecap="round" stroke-linejoin="round"/></svg>
<div>{{- $group.title -}}</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be just a span @mdo?

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it doesn't matter because the parent element is a flex container and will align everything essentially the same way.

XhmikosR
XhmikosR commented Jul 17, 2019
View reviewed changes
site/layouts/partials/docs-sidebar.html
@@ -38,7 +37,9 @@

<li class="my-3 mx-4 border-top"></li>
<li class="bd-sidenav-group pl-3">
<a class="d-inline-flex bd-sidenav-group-link" href="/migration">Migration</a>
<a class="d-inline-flex align-items-center bd-sidenav-group-link" href="/migration">
<div>Migration</div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't this be in nav.yml?

@XhmikosR
Copy link
Member Author

Alright, I left a couple more comments, but I'd like them addressed soon so that we don't end up with a huge PR.

@XhmikosR
Copy link
Member Author

I'm putting this on hold because I believe it can be optimized further.

  1. We shouldn't use an a tag for the group item since it's basically a button
  2. Maybe we could just use our collapse plugin here?

@XhmikosR XhmikosR force-pushed the master-xmr-docs branch 2 times, most recently from 74b9710 to 6929fb1 Compare July 17, 2019 12:29
@XhmikosR XhmikosR removed the on-hold label Jul 17, 2019
@XhmikosR XhmikosR merged commit 4634fd7 into master Jul 17, 2019
@XhmikosR XhmikosR deleted the master-xmr-docs branch July 17, 2019 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartijnCuppens MartijnCuppens MartijnCuppens approved these changes

@mdo mdo Awaiting requested review from mdo

Assignees
No one assigned
Labels
docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XhmikosR @patrickhlauke @mdo @MartijnCuppens