Conversation
daniel-montalvo
left a comment
There was a problem hiding this comment.
Hi @G0dwin .
thanks for putting this together.
I think the act-tools repo should be the place to have this conversation, as this is were the logic lives.
I have a couple of thoughts based on previous work, especially act-rules/act-rules.github.io#2377 and w3c/wcag-act-rules#387
The URIs we settled on for these examples were: /act/rules/terms/accessible-name/examples/
This is because on the WAI website we have the pattern that every URI needs to be meaningful in itself.
If we include this work as-is, we would leave /act/rules/terms/ with no meaningful content, and we would be adding yet another pattern /act/rules/glossary/ which wouldn't meet user expectations.
I propose we rename /act-rules/glossary/ to /act/rules/terms/, just to avoid having two patterns repeated throughout.
In addition, I'd request review from @remibetin because these code updates are intended to push content to the WAI website, so he needs to be aware as the WAI website maintainer.
Thanks Daniel, I updated the URL and committed the change. You can preview it here. |
|
Thanks @daniel-montalvo and @G0dwin. I'll review the use of the WAI theme, and I've asked @kfranqueiro if he could review the Typescript updates. We plan to coordinate next week on this. |
remibetin
left a comment
There was a problem hiding this comment.
Hi @G0dwin,
Thanks for your work on this. I've started the review and added some suggestions and questions.
To help with the review, I'm interested in more information on who the glossary page is intended for, and examples of use cases. AFAIK, each rule already includes a list of glossary terms with their definition. Will there be links to the glossary page somewhere else?
I also think the scope and purpose of the glossary page is important to convey to the end-users. For example, users may wonder how this relates to section 4.12 Glossary of ACT Rules Format 1.1. To that end, I suggest adding a "Summary" box at the top that provide this information. I can help refine the wording.
| lines.push("### Used in rules"); | ||
|
|
||
| const rules = [...(usedInRules.get(key) || new Set())].sort((a, b) => | ||
| a.id.localeCompare(b.id), | ||
| ); | ||
| if (rules.length === 0) { | ||
| lines.push("- None"); | ||
| } else { | ||
| rules.forEach((rule) => { | ||
| lines.push( | ||
| `- [${rule.name}](/standards-guidelines/act/rules/${rule.id}/proposed/)`, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The "Used in Rules" list can become quite big and require a lot of scrolling from users. For example, the list for the "Outcome" term consists of 94 items.
Before considering possible solutions, could you clarify who this section is intended for?
There was a problem hiding this comment.
I agree, this adds a large amount of visual space. I think embedding this content in summary and details elements is probably the best solution but I that would be assuming use cases myself. This came from the issue requirements:
In scope: New glossary index (and/or per-term pages), navigation entry, working anchors and links on WAI, “used in rules” lists with correct WAI rule URLs, full definition bodies as above.
I think it may also be a good option to move forward with the page as it is and adjust this later if it holds us up.
@WilcoFiers , do you have any context or suggestions to add here?
Co-authored-by: Rémi Bétin <github@remibetin.fr>
Co-authored-by: Rémi Bétin <github@remibetin.fr>
Co-authored-by: Rémi Bétin <github@remibetin.fr>
Co-authored-by: Rémi Bétin <github@remibetin.fr>
Co-authored-by: Rémi Bétin <github@remibetin.fr>
WilcoFiers
left a comment
There was a problem hiding this comment.
Looks good. Couple nitpicks.
There was a problem hiding this comment.
Would you mind adding some tests for this?
There was a problem hiding this comment.
Tests for src/utils/glossary.ts were added in commit 6aa3ade, covering getGlossaryHeading, normalizeHeadingLevels, and getGlossaryBody (both rule and full modes).
| export const getFullGlossary = (glossary: DefinitionPage[]): string => { | ||
| const glossaryTexts = glossary.map(getFullGlossaryMarkdown); | ||
| return joinStrings(`## Glossary`, ...glossaryTexts); | ||
| }; |
There was a problem hiding this comment.
Are getFullGlossary and getFullGlossaryMarkdown just for testing? Seems unnecessary if that's the case. If we want to test the mode of getGlossaryBody we should test that function directly.
There was a problem hiding this comment.
Great catch, I'll remove it.
There was a problem hiding this comment.
getFullGlossary and getFullGlossaryMarkdown were removed in commit 6aa3ade.
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: G0dwin <1109252+G0dwin@users.noreply.github.com>
Resolved the merge conflict with |
Adds the tools necessary to generate a glossary page and add it to the nav.
Dependent PR in
act-rulesto be published shortly.Note: much of this was generated using Co-pilot, be on the lookout for code that doesn't meet standards.
Because we probably will not be able to preview this until after we merge the packages, I have a preview available on my personal site here.
Related PR: act-rules/act-rules.github.io#2395