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

120 skiplinks b #307

Merged
merged 8 commits into from
Mar 6, 2019
Merged

120 skiplinks b #307

merged 8 commits into from
Mar 6, 2019

Conversation

JBCSU
Copy link
Collaborator

@JBCSU JBCSU commented Mar 5, 2019

No description provided.

@JBCSU JBCSU requested review from josephgknox and sherakama March 5, 2019 20:33
Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

You definitely put in the effort for this one. I'm totally ok with splitting this up in to separate templates as it gives a good 'working example' of how it can be done. A couple of comments for refinement however:

  • Twig template names. Can we remove the underscore as that is not a twig convention but only a SASS convention?
  • Can we restore the anchor naming work that Yvonne and Joe had done previously?
  • The skip to content options that I have seen before, jump to an anchor on a div instead of into a heading. Not having actually tested this, I would imagine that a screen reader's behavior would differ if we jumped to a heading and I am not sure the repercussions of this. Something to ask Jiatyan.

@JBCSU
Copy link
Collaborator Author

JBCSU commented Mar 5, 2019

You definitely put in the effort for this one. I'm totally ok with splitting this up in to separate templates as it gives a good 'working example' of how it can be done. A couple of comments for refinement however:

  • Twig template names. Can we remove the underscore as that is not a twig convention but only a SASS convention?

Oh, yeah, sure. Sorry, I thought I had seen twig files named that way.

  • Can we restore the anchor naming work that Yvonne and Joe had done previously?

I don't think so, because:

  • the id is now used in 2 separate templates,
  • it needs to have the same default in both, and
  • in normal usage the rollup .twig file will not be used, so we can't set the shared default there.

I therefore picked an ID I thought would be unique but not bizarre: su- to match decanter naming conventions; main-content, because that's semantically descirptive; 1891 is the year Stanford was founded. We can pick a different default, but I think we have to actually hardcode a default because of the above-stated reasons.

  • The skip to content options that I have seen before, jump to an anchor on a div instead of into a heading. Not having actually tested this, I would imagine that a screen reader's behavior would differ if we jumped to a heading and I am not sure the repercussions of this. Something to ask Jiatyan.

It's a heading so the screen reader will actually read it. We had this discussion for homesite, and that's why homesite uses an <h2>.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Sounds like you have done your due diligence on this behavior. Good to me.

@sherakama sherakama requested a review from yvonnetangsu March 6, 2019 00:30
@sherakama sherakama added this to the Version 5.0.0 milestone Mar 6, 2019
@josephgknox
Copy link

@yvonnetangsu you good with this, too? :)

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the tweaks! 👍

@sherakama sherakama merged commit 67221eb into 120-skiplinks Mar 6, 2019
@sherakama sherakama deleted the 120-skiplinks-b branch March 6, 2019 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants