-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add tabindex and focus states to code blocks #215
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richardTowers
force-pushed
the
tabbable-code-blocks
branch
from
March 11, 2021 14:45
c0932e8
to
67994d8
Compare
36degrees
reviewed
Mar 11, 2021
richardTowers
force-pushed
the
tabbable-code-blocks
branch
from
March 11, 2021 15:35
67994d8
to
0f5303b
Compare
36degrees
reviewed
Mar 11, 2021
36degrees
approved these changes
Mar 11, 2021
This is required for accessibility reasons. Because some code blocks have scrollbars they need to be focusable using a keyboard. I've copied the styles from the design system's documentation (see the code blocks in the HTML / Nunjucks tabs here: https://design-system.service.gov.uk/components/button/) The implementation is simple when syntax highlighting is not enabled, because we can simply implement `block_code` to surround the code with the markup we want. When syntax highlighting is enabled it's a bit more complicated. Middleman already `include`s its own implementation of `block_code`, which renders the source code highlighted with spans. This method doesn't look like it would be easy to customise, so instead I've resorted to post-processing the HTML (i.e. replacing the attributes on the `<pre>` tag using a regex). Testing this was a bit tricky, because you need to monkey patch the class to include the `block_code` method. I had to clone the class and patch the clone to do avoid affecting other tests.
This doesn't change the CSS generated, but makes it a bit clearer why the padding is 13px in the focus state and 15px otherwise. It should also make it easier to keep in sync with future changes to padding / borders.
We don't need to call processor.render in every test, as it's always called with the same arguments. That means it can be pulled out into a `let` block. Moving the code block tests into the top level describe lets us use described_class in the tests, which cuts down on a bit of syntax noise. We can get a bit clever by only replacing `processor` and not `output` in the "with syntax highlighting" case - the test will use the `output` block from `describe "#render a code block"`, but with the `processor` from `describe "with syntax highlighting"`
richardTowers
force-pushed
the
tabbable-code-blocks
branch
from
March 12, 2021 13:54
6467c8a
to
e11be57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Just a note on the test organisation and semantic. It would be easier to follow with a couple of minor changes:
- only one
describe
block to describe what you're testing - two
context
blocks to specify the conditions of the tests
It would look something like this:
describe "#render a code block" do
context "without syntax highlighting" do
# setup for conditions without syntax highlighting
it "sets tab index to 0"
it "renders the code without syntax highlighting"
context "with syntax highlighting" do
# setup for conditions with syntax highlighting
it "sets tab index to 0"
it "renders the code with syntax highlighting"
end
end
- one describe block to describe what you're testing - context blocks to specify the conditions of the tests
Thanks! Addressed in a0c9bfd |
AlanGabbianelli
approved these changes
Mar 12, 2021
3 tasks
This was referenced Mar 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is required for accessibility reasons. Because some code blocks have scrollbars they need to be focusable using a keyboard.
I've copied the styles from the design system's documentation (see the code blocks in the HTML / Nunjucks tabs here: https://design-system.service.gov.uk/components/button/)
The implementation is simple when syntax highlighting is not enabled, because we can simply implement
block_code
to surround the code with the markup we want.When syntax highlighting is enabled it's a bit more complicated. Middleman already
include
s its own implementation ofblock_code
, which renders the source code highlighted with spans. This method doesn't look like it would be easy to customise, so instead I've resorted to post-processing the HTML (i.e. replacing the attributes on the<pre>
tag using a regex).Testing this was a bit tricky, because you need to monkey patch the class to include the
block_code
method. I had to clone the class and patch the clone to do avoid affecting other tests.Example from a locally running version of the paas tech docs: