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

feat: Add gcds-notice documentation #442

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

ethanWallace
Copy link
Collaborator

@ethanWallace ethanWallace commented Dec 4, 2024

Summary | Résumé

Add documentation for the gcds-notice component.

To-do

  • Fill in component preview box in base files.
  • Use version of @cdssnc/gcds-components with published gcds-notice.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-442.djtlis5vpn8jd.amplifyapp.com

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-442.d35vdwuoev573o.amplifyapp.com

@ethanWallace ethanWallace marked this pull request as ready for review December 12, 2024 13:16
Copy link
Collaborator

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

Added a few small comments :)


### Define the type of notice

Use the `type` attribute to categorize the notice as ‘Info’, ‘Warning’, ‘Danger’ or ‘Success’. This choice determines the visual styling of the notice and communicates the urgency or importance of the message to people.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we flag ‘Info’, ‘Warning’, ‘Danger’ or ‘Success’ as code and make it lowercase to match the values required to pass to the type prop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like this idea.

@SmartMouthWords what do you think about this change?

Copy link
Collaborator

@daine daine Dec 12, 2024

Choose a reason for hiding this comment

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

Here's what we've done especially when it involves getting the french translation for some items that are best left as code:

Suggested change
Use the `type` attribute to categorize the notice as Info’, ‘Warning’, ‘Danger or Success. This choice determines the visual styling of the notice and communicates the urgency or importance of the message to people.
Use the `type` attribute to categorize the notice as `info` (Info), `warning` (Warning), `danger` (Danger) or `Success` (Success). This choice determines the visual styling of the notice and communicates the urgency or importance of the message to people.

Here is a preview:

Use the type attribute to categorize the notice as info (Info), warning (Warning), danger (Danger) or Success (Success). This choice determines the visual styling of the notice and communicates the urgency or importance of the message to people.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@daine can we also change Success to lowercase please to match the code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at it again - can we skip the text in brackets for EN since it's the same word?

### Specify the notice title and heading level

- Use the `notice-title` attribute to create a clear and informative heading for the notice. Ensure the title conveys the message’s purpose.
- Set the `notice-title-tag` to define the correct heading level for the card title. Follow a correct and orderly heading hierarchy to make it equal for people who use assistive technologies.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we link correct heading level to this section on the heading component guidance page? Hopefully this will help users pick the correct level.

src/en/components/notice/use-case.md Outdated Show resolved Hide resolved
src/fr/composants/avis/cas-dutilisation.md Outdated Show resolved Hide resolved

### Définir le type d’avis

Utilisez l’attribut `type` pour classer l’avis dans les catégories « Information », « Avertissement », « Danger » ou « Succès ». Ce choix détermine le style visuel de l’avis et communique l’urgence ou l’importance du message au public.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comment about: « Information », « Avertissement », « Danger » ou « Succès ».

Can we flag them as code, have them in lowercase and keep them in English so they match the actual values users need to pass to the component?

Copy link
Collaborator

@daine daine Dec 12, 2024

Choose a reason for hiding this comment

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

Does this work for the french text @SmartMouthWords ?

Suggested change
Utilisez l’attribut `type` pour classer l’avis dans les catégories « Information », « Avertissement », « Danger » ou « Succès ». Ce choix détermine le style visuel de l’avis et communique l’urgence ou l’importance du message au public.
Utilisez l’attribut `type` pour classer l’avis dans les catégories `info` (Information), `warning` (Avertissement), `danger` (Danger) ou `success` (Succès). Ce choix détermine le style visuel de l’avis et communique l’urgence ou l’importance du message au public.

Here is an preview:

Utilisez l’attribut type pour classer l’avis dans les catégories info (Information), warning (Avertissement), danger (Danger) ou success (Succès). Ce choix détermine le style visuel de l’avis et communique l’urgence ou l’importance du message au public.

### Précisez le titre et le niveau de l’en-tête de l’avis

Utilisez l’attribut `notice-title` pour créer un titre clair et informatif pour l’avis. Veillez à ce que le titre transmette l’objectif du message.
Utilisez `notice-title-tag` pour définir le bon niveau de titre pour la carte. Suivez une hiérarchie correcte et ordonnée pour fournir une expérience équitable aux personnes utilisant des technologies d’assistance.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment about the heading level link.

## Structure de la Avis

<ol class="anatomy-list">
<li><strong>list</strong></li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops found one more - can we remove this list item?

Copy link
Collaborator

@daine daine left a comment

Choose a reason for hiding this comment

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

I found this behaviour, which might be from the component itself.

Changing the title tag Changing the `notice-title-tag`, on the code, looks fine, but the rendered title does not look correct. Here's a video of me changing it and not seeing a visual change to the title.
Screenshare.-.2024-12-12.2_25_07.PM.mp4

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.

Documentation: Notice component
3 participants