Skip to content

Doc tweaks#29029

Merged
XhmikosR merged 9 commits intomasterfrom
master-xmr-docs
Jul 17, 2019
Merged

Doc tweaks#29029
XhmikosR merged 9 commits intomasterfrom
master-xmr-docs

Conversation

@XhmikosR
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

patrickhlauke commented Jul 16, 2019

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

yes please

@mdo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread site/layouts/partials/docs-sidebar.html
<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
Copy Markdown
Member Author

@XhmikosR XhmikosR Jul 17, 2019

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
Copy Markdown
Member

@mdo mdo Jul 18, 2019

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
Comment thread site/layouts/partials/docs-sidebar.html Outdated
Copy link
Copy Markdown
Member Author

@XhmikosR XhmikosR Jul 17, 2019

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
Copy Markdown
Member Author

XhmikosR commented Jul 17, 2019

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
Copy Markdown
Member Author

XhmikosR commented Jul 17, 2019

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