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

Enable focus + keyboard scroll on Markdown code blocks #2566

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jan 30, 2023

This PR consolidates styling between <pre> and <code>

All code blocks can now receive focus and scroll with keyboard left/right arrows

Code snippet with keyboard focus and arrow key scrolling

@colinrotherham colinrotherham added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) Frontend squad labels Jan 30, 2023
@colinrotherham colinrotherham added this to Needs review 🔍 in Design System Sprint Board via automation Jan 30, 2023
@netlify
Copy link

netlify bot commented Jan 30, 2023

You can preview this change here:

Name Link
🔨 Latest commit b860e15
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/63f62a817167070008fad578
😎 Deploy Preview https://deploy-preview-2566--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -37,7 +37,7 @@ nodeListForEach($tabs, function ($tabs) {
OptionsTable.init()

// Add copy to clipboard to code blocks inside tab containers
var $codeBlocks = document.querySelectorAll('[data-module="app-copy"]')
var $codeBlocks = document.querySelectorAll('[data-module="app-copy"] pre')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now render code through Markdown so I've moved data-module to the wrapper, otherwise every code block on every page will get an unintentional Copy code button

margin-bottom: 0;
padding: 0;
border: $govuk-focus-width solid transparent;
.app-tabs__container pre code {
Copy link
Contributor Author

@colinrotherham colinrotherham Jan 30, 2023

Choose a reason for hiding this comment

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

font-size: 16px;
@include govuk-responsive-margin(4, "bottom");

&:focus {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the focus styles from .app-tabs__container pre so all blocks are consistent:

Before

No keyboard focus or scroll

Code snippet without keyboard focus or arrow key scrolling

After

Code snippet with keyboard focus and arrow key scrolling

Base automatically changed from markdown-render-order to main January 31, 2023 16:09
@colinrotherham colinrotherham changed the title Render all code blocks using Markdown to avoid bug Move code keyboard focus/scroll to <code> not <pre> Feb 1, 2023
@colinrotherham colinrotherham changed the title Move code keyboard focus/scroll to <code> not <pre> Enable focus + keyboard scroll on all code blocks Feb 1, 2023
@colinrotherham colinrotherham added the Enhancement: feature-request User requests a new feature label Feb 15, 2023
@36degrees 36degrees self-requested a review February 21, 2023 14:18
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

One comment that could do with being updated, but otherwise looks good to me. Good catch updating the code blocks in 'Updating your code' too 👍🏻

Comment on lines 4 to 5
* Custom markdown renderer that avoids wrapping `<img>` image
* tags (or code with syntax highlighting) in `<p>` paragraphs
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right any more – as far as I know the code method has nothing to do with avoiding the code blocks being wrapped in paragraphs?

Suggest either adding comment blocks to the individual methods or changing to something like:

Suggested change
* Custom markdown renderer that avoids wrapping `<img>` image
* tags (or code with syntax highlighting) in `<p>` paragraphs
* Custom markdown renderer that:
* - avoids wrapping `<img>` image tags in `<p>` paragraphs
* - adds tabindex="0" to code blocks so they can be focused and scrolled with
* the keyboard

Copy link
Contributor Author

@colinrotherham colinrotherham Feb 21, 2023

Choose a reason for hiding this comment

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

Ahh let me dig into why I adjusted the comment

I found that "new lines" in code blocks were triggering another .code() call as "code within code"

The first .code() call gets <pre><code> and has a syntax language set
Subsequent .code() calls skip straight to .html() and don't have a syntax language

Hopefully it's something we fixed (or will fix) in:

We might both be right

Copy link
Contributor Author

@colinrotherham colinrotherham Feb 21, 2023

Choose a reason for hiding this comment

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

Yeah it was related new lines so the original comment has been moved to:

This PR is now stacked on top and adds the comment:

// Ensure code blocks can be focused and scrolled
// with the keyboard via `tabindex="0"`

@colinrotherham colinrotherham force-pushed the code-syntax-scrolling branch 2 times, most recently from a6431f8 to 9fa400b Compare February 21, 2023 20:52
@colinrotherham colinrotherham changed the base branch from main to code-syntax-markdown February 21, 2023 21:40
This change stops every Markdown-rendered code block (on every page) getting an unintentional “Copy code” button
@colinrotherham colinrotherham changed the base branch from code-syntax-markdown to main February 22, 2023 12:16
@colinrotherham colinrotherham changed the title Enable focus + keyboard scroll on all code blocks Enable focus + keyboard scroll on Markdown code blocks Feb 22, 2023
@colinrotherham
Copy link
Contributor Author

@36degrees I've adjusted the PR title to show that scrolling only works on Markdown-rendered code blocks

Code blocks rendered by Nunjucks won't scroll until this next one is merged:

Hope that's alright

*/
code (code, language, isEscaped) {
return super.code(code, language, isEscaped)
.replace('<code', '<code tabindex="0"')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ensure we add tabindex="0" to Markdown code blocks

This maintains keyboard focus + arrow key scrolling as added in:

Fixes issue where “inline code” styles (see `updating-your-code` how to guide) was conflicting

Also consistent with `highlight.js` which now scrolls code not the preformatted text
@colinrotherham colinrotherham merged commit d6e001b into main Feb 22, 2023
@colinrotherham colinrotherham deleted the code-syntax-scrolling branch February 22, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) Enhancement: feature-request User requests a new feature
Projects
Design System Sprint Board
  
Needs review 🔍
Development

Successfully merging this pull request may close these issues.

None yet

3 participants