Fix generating example IDs #813
Conversation
This reverts commit dc6c2c4.
|
You can preview this change here: Built with commit ba39de7 https://deploy-preview-813--govuk-design-system-preview.netlify.com |
|
What was the issue with the previous version? This version relies on the 'titleSuffix' being a single word so will break as well I think... |
|
Looks like it was an issue with titles like https://design-system.service.gov.uk/styles/typography/captions/index.html since they have endashes |
|
Yeah the dashes 👍 I wrote up an incident report. That's a good point about the I've also just realised that I meant to add some error catching to the JS that broke this so need to do that. |
This reverts a hotfix #809 that removed the changes in favour of a proper fix for #620 Generating example IDs from example titles can be brittle if there are unexpected characters in titles. Use slugger for more robust converting of exampleTitle to exampleId. This also uses the solution from #620 to fix duplicate IDs by adding an extra parameter "titleSuffix" to examples if necessary. Co-authored-by: Nick Colley <[email protected]>
hannalaakso
force-pushed
the
fix-axe-tests-pr
branch
2 times, most recently
from
fd5700a
to
48fcf34
Compare
|
Thanks a lot @NickColley I think those look like sensible suggestions 👍 I've also added some error catching to tabs.js that would have caught these errors and shown a console error instead. |
src/javascripts/components/tabs.js
Outdated
| try { | ||
| $tabContainer = this.$module.querySelector($currentToggler.hash) | ||
| } catch (exception) { | ||
| if ((typeof console !== 'undefined' || typeof console.log !== 'undefined')) { |
There was a problem hiding this comment.
Does this swallow the exception?
Potentially this would make it less likely for us to be able to catch bugs since it would not break the build?
If we think it's useful to have a more helpful error message and that is true, we could consider throwing a custom error here rather than just logging it to the console?
There was a problem hiding this comment.
A good catch, thanks @NickColley
src/javascripts/components/tabs.js
Outdated
| // make current active | ||
| $tabContainer.classList.remove(tabContainerHiddenClass) | ||
| $tabContainer.setAttribute('aria-hidden', 'false') | ||
| if ($tabContainer) { |
There was a problem hiding this comment.
To avoid unnecessary nesting you could consider returning early here, although I haven't seen the rest of the code so might not be possible :)
There was a problem hiding this comment.
Hmmm my reasoning was that it's often better to stick to a single return point to make the code more readable. But I think in this case the readability increased with your equally valid suggestion so have changed it. Thanks 🙂
| {% endif %} | ||
| {% set exampleTitle = exampleTitle + " example" %} | ||
|
|
||
| {% if params.id %} |
There was a problem hiding this comment.
I don't think there are any examples that pass through 'id' now so this may be unnecessary.
There was a problem hiding this comment.
id is still used by at least header and footer examples.
There was a problem hiding this comment.
I wonder if it's necessary though given that we can generate a unique id from the unique title?
Not blocking just thought this could simplify the example partial a bit
hannalaakso
force-pushed
the
fix-axe-tests-pr
branch
from
48fcf34
to
ba39de7
Compare
This reverts a hotfix #809 that removed the changes, in favour of a fix for generating example IDs for #784.
Generating example IDs from example titles can be brittle if there are unexpected characters in titles. This goes back to using
exampleTitleto generate the IDs with slugger and uses the solution from #620 to fix duplicate IDs that were flagged by Axe. Alternatively we could keep the kebab casing filter and try to make sure that the Regex in it can cope with foreign characters but it seems kind of liable to breaking.If someone adds the same example to a page several times, they'll need to specify
titleSuffixfor the example macro asexampleparameter from the macro won't be unique but this issue existed also with the previous solution. We could think about testing all the pages to catch this.Best just to review these commits directly: