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

fix(documentation): repeated icon note #1027

Closed
wants to merge 3 commits into from
Closed

fix(documentation): repeated icon note #1027

wants to merge 3 commits into from

Conversation

olaf-k
Copy link
Contributor

@olaf-k olaf-k commented Feb 13, 2023

there is an a11y note warning authors for using only an icon repeated in several components. this PR replaces it with a small include.
This PR:

  • adds the note to accordion-item and the option.
  • corrects the badge note (which was truncated) and the card one (which was wrong).
  • removes the "Icon only" paragraph of nav-disclosure for consistency (we don't do it for other widgets).

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Merging #1027 (e2d3f7f) into main (d61b119) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main     #1027    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          123       222    +99     
  Lines         1562      2406   +844     
  Branches       108       125    +17     
==========================================
+ Hits          1562      2406   +844     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libs/components/src/lib/badge/badge.ts 100.00% <ø> (ø)
libs/components/src/lib/button/button.template.ts 100.00% <ø> (ø)
libs/components/src/lib/fab/fab.template.ts 100.00% <ø> (ø)
libs/components/src/lib/header/header.template.ts 100.00% <ø> (ø)
libs/components/src/lib/layout/layout.ts 100.00% <ø> (ø)
.../src/lib/accordion-item/accordion-item.template.ts 100.00% <100.00%> (ø)
...omponents/src/lib/accordion-item/accordion-item.ts 100.00% <100.00%> (ø)
...bs/components/src/lib/accordion-item/definition.ts 100.00% <100.00%> (ø)
libs/components/src/lib/accordion-item/index.ts 100.00% <100.00%> (ø)
...components/src/lib/accordion/accordion.template.ts 100.00% <100.00%> (ø)
... and 179 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -0,0 +1 @@
Note: An icon on its own doesn't make a discernible text. An `aria-label`, `aria-labelledby` or `title` must be provided to ensure that the user can understand the component's purpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

When the icon is followed by with text and has no real need for screen readers than authors should add aria-hidden="true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, but that's valid for every bit of text in an app, not just when using an icon.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand..

Copy link
Contributor

Choose a reason for hiding this comment

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

For SVG, If no explicit role/aria set, it's hidden by default. Is it not?

Copy link
Contributor

Choose a reason for hiding this comment

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

no.....

Copy link
Contributor

Choose a reason for hiding this comment

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

When the icon is followed by with text and has no real need for screen readers than authors should add aria-hidden="true"

the sentence starts with "An icon on its own..."

Copy link
Contributor

@rachelbt rachelbt left a comment

Choose a reason for hiding this comment

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

nice docs maintaining touch-up!
How come there are new snapshots for the nav-discloser? seems not relevant to this PR...

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 13, 2023

nice docs maintaining touch-up! How come there are new snapshots for the nav-discloser? seems not relevant to this PR...

you're right, this should have been a doc only PR. but since I updated the nav-disclosure doc to remove a paragraph, it shows in the ui tests!

@rachelbt
Copy link
Contributor

nice docs maintaining touch-up! How come there are new snapshots for the nav-discloser? seems not relevant to this PR...

you're right, this should have been a doc only PR. but since I updated the nav-disclosure doc to remove a paragraph, it shows in the ui tests!

why did you removed it? don't we have an icon-only option?

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 14, 2023

why did you removed it? don't we have an icon-only option?

this was the nav-disclosure comment for the paragraph I removed:
image
as I wrote, I did it for consistency purposes: there are other widgets that allow authors to inject an icon while not providing text/label (accordion-item, badge, button, etc.) and we don't provide a specific example for them.

@yinonov
Copy link
Contributor

yinonov commented Feb 14, 2023

Is this supported in GH markdowns?

@yinonov
Copy link
Contributor

yinonov commented Feb 14, 2023

Is this supported in GH markdowns?

No
image

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 14, 2023

Is this supported in GH markdowns?

No

of course, that's nunjucks syntax, markdown is not a templating language. is that a dealbreaker? do you expect users to read the doc on GH?

@yinonov
Copy link
Contributor

yinonov commented Feb 14, 2023

Is this supported in GH markdowns?

No

of course, that's nunjucks syntax, markdown is not a templating language. is that a dealbreaker? do you expect users to read the doc on GH?

Yes. Otherwise we would have wrote directly in 11ty templates

See

whatwg/html#8287

And

whatwg/html#6507 (comment)

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 14, 2023

Yes. Otherwise we would have wrote directly in 11ty templates

that's (partly) the point of this PR: we have repeating information and a templating engine, why not take advantage of it?

@yinonov
Copy link
Contributor

yinonov commented Feb 14, 2023

Yes. Otherwise we would have wrote directly in 11ty templates

that's (partly) the point of this PR: we have repeating information and a templating engine, why not take advantage of it?

because you lose documentation in GitHub. the closest you can get to mirroring content in GH is via permalinks. where in 11ty this type of url should be intercepted and replaced with the actual mirrored content. but that windowed content can still be somewhat confusing

Design tokens are all the values needed to construct and maintain a design system — color, typography, spacing, object styles, sizing, etc. — represented as data.
These can represent anything defined by design: font size in pixel, a color as a RGB value, an opacity as a number, etc. They’re used in place of hard-coded values in order to ensure flexibility and unity across all product experiences.
Design tokens are directly integrated into our component libraries and UI kits. They cover the various options of platform scales, color themes, component states, and more.
Using design tokens allows us to manage and maintain our design system as Vonage’s design Single source of truth.

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 15, 2023

tigger-sad

@olaf-k
Copy link
Contributor Author

olaf-k commented Feb 15, 2023

include reverted and replaced by copy/paste, first post updated.

Copy link
Contributor

@yinonov yinonov left a comment

Choose a reason for hiding this comment

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

this caveat would make more sense when the documentation flow in this order

  1. text / label section
  2. icon section (showing icon only without text / label). in the icon section it makes sense to explain about the caveat sense it's a real case
  3. icon with text / label

also bear in mind to apply those same rules in the code snippet

@rinaok rinaok closed this Mar 22, 2023
@rinaok rinaok deleted the doc-icon-note branch July 10, 2023 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants