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(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert as a component #1464

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

Tlahey
Copy link
Contributor

@Tlahey Tlahey commented Sep 28, 2023

Changes description

image

Svelte part :

  • Export the VtmnAlertItem component in order to be used without the VtmnAlert.
  • Title / description can be set with a slot
  • Change the <p> to <span> in order to be ISO with other frameworks

Svelte / React / Vue :

  • Add a new property ariaLabelCloseButton
  • Remove the id no really necessary
  • Set the timeout property to 0 in order to keep the alert visible

Also checked https://www.w3.org/WAI/ARIA/apg/patterns/alertdialog/examples/alertdialog/
The div with a role alert doesn't need to have a aria-labelledby /describeby
image

Context

Checklist

  • Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
  • Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
  • Check your code additions will fail neither code linting checks.
  • I have reviewed the submitted code.
  • I have tested on related showcases.
  • If it includes design changes, please ask for a review design-system-core-team-design GitHub team.

Does this introduce a breaking change?

  • Yes
  • No (checked on the DKT project, the id is not used)

Other information

@Tlahey Tlahey requested review from thibault-mahe and a team as code owners September 28, 2023 07:52
@Tlahey Tlahey changed the title feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert is used as a component feat(@vtmn/svelte, @vtmn/react, vtmn/vue): use VtmnAlert as a component Sep 28, 2023
Copy link
Member

@lauthieb lauthieb left a comment

Choose a reason for hiding this comment

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

Hi @Tlahey, thanks for this PR.

Regarding potential breaking changes, did you check elsewhere than the DKT project but in all repositories of our private GitHub organization?

The only warning I imagine it's about that Alert is an overlay in our design system, (and it is in this "Overlays" category), and here we are allowing to use it also inside interfaces (not overlayed).

https://github.com/orgs/Decathlon/teams/design-system-core-team-design @Sabrinavigil @SimonLeclercq what's your opinion? Will this change also have an impact on the design and documentation of overlay semantics?

@thibault-mahe what's your opinion please regarding these changes?

Thank you very much 🙏

thibault-mahe
thibault-mahe previously approved these changes Sep 28, 2023
@thibault-mahe
Copy link
Contributor

hi! thanks for this PR @Tlahey and for pinging me @lauthieb . Everything is ok for me here, the use of the slot is nice and allows the component to be more flexible, without bringing any breaking change. The only mini breaking change is that the aria-label of the close button is no longer set to "Close toast" by default, which you can quickly change if you want @Tlahey.

Okay fo me otherwise.

@Tlahey
Copy link
Contributor Author

Tlahey commented Sep 29, 2023

Hello @thibault-mahe I've done the fix on the aria-label ;)

@Tlahey Tlahey merged commit 590dd2e into main Sep 29, 2023
4 checks passed
@Tlahey Tlahey deleted the feat/vtmnAlertItem branch September 29, 2023 09:28
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.

3 participants