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

BREAKING CHANGE: The alert role has been removed for Alert component #DS-1175 #1398

Merged
merged 3 commits into from
May 10, 2024

Conversation

curdaj
Copy link
Contributor

@curdaj curdaj commented May 6, 2024

Description

  • Add role="alert" to ValidationText

Additional context

Issue reference

@github-actions github-actions bot added the BC Breaking change label May 6, 2024
Copy link

netlify bot commented May 6, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit a1e56f5
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/663dfaf361e97b0008a8b8cf
😎 Deploy Preview https://deploy-preview-1398--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 93 (no change from production)
Best Practices: 100 (no change from production)
SEO: 82 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@coveralls
Copy link

coveralls commented May 6, 2024

Coverage Status

coverage: 80.418% (-16.0%) from 96.371%
when pulling a1e56f5 on bc/ds-1175-accessibility-for-alert
into 617b507 on integration/release-v2.

Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

Good Job 👍

@literat literat self-requested a review May 6, 2024 14:23
@literat
Copy link
Collaborator

literat commented May 6, 2024

Good Job 👍

Sorry, still a draft, but it is looking good.

@curdaj curdaj marked this pull request as ready for review May 6, 2024 15:12
@curdaj curdaj requested review from adamkudrna, crishpeen, pavelklibani and a team as code owners May 6, 2024 15:12
@curdaj curdaj requested a review from crishpeen May 7, 2024 09:13
docs/migrations/web-twig/MIGRATION-v3.md Outdated Show resolved Hide resolved
docs/migrations/web-react/MIGRATION-v2.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@curdaj curdaj force-pushed the integration/release-v2 branch from 6212b9d to 6bafe41 Compare May 9, 2024 09:31
@curdaj curdaj force-pushed the bc/ds-1175-accessibility-for-alert branch from e9d31ed to a78c9e1 Compare May 9, 2024 09:37
@curdaj curdaj requested review from crishpeen and literat May 9, 2024 09:48
Copy link
Collaborator

@literat literat left a comment

Choose a reason for hiding this comment

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

Nice! 👍

@curdaj curdaj force-pushed the bc/ds-1175-accessibility-for-alert branch from a3c246b to 041ba6d Compare May 9, 2024 13:00
@adamkudrna
Copy link
Contributor

As per our contributing guidelines, the commit message should go like this:

BREAKING CHANGE: Remove the `role` attribute from the `Alert` component #DS-1175

or

BREAKING CHANGE: Remove the `role` attribute from `Alert` #DS-1175

Copy link
Contributor

@adamkudrna adamkudrna left a comment

Choose a reason for hiding this comment

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

👏🏻

#### Migration Guide

In cases where you need to use the component as an information that requires the user's immediate attention,
you can use `role="alert"` as an [additional attributes][readme-additional-attributes].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
you can use `role="alert"` as an [additional attributes][readme-additional-attributes].
you can use `role="alert"` as an [additional attribute][readme-additional-attributes].

#### Migration Guide

In cases where you need to use the component as an information that requires the user's immediate attention,
you can use `role="alert"` as an [additional attributes][readme-additional-attributes].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
you can use `role="alert"` as an [additional attributes][readme-additional-attributes].
you can use `role="alert"` as an [additional attribute][readme-additional-attributes].

Comment on lines 47 to 48
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In cases where you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attributes][readme-additional-attributes].
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻

Suggested change
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In cases where you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attributes][readme-additional-attributes].
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In case you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attribute][readme-additional-attributes].


#### Migration Guide

In cases where you need to use the component as an information that requires the user's immediate attention,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In cases where you need to use the component as an information that requires the user's immediate attention,
In case you need to use the component as an information that requires the user's immediate attention,


#### Migration Guide

In cases where you need to use the component as an information that requires the user's immediate attention,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In cases where you need to use the component as an information that requires the user's immediate attention,
In case you need to use the component as an information that requires the user's immediate attention,

Comment on lines +57 to +58
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In cases where you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attributes][readme-additional-attributes].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In cases where you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attributes][readme-additional-attributes].
⚠️ Please pay attention to the accessibility setting when Alert is dynamically displayed. In case you need to use the component as an information that requires the user's immediate attention,
you can set `role="alert"` as an [additional attribute][readme-additional-attributes].

Comment on lines +19 to +21
<Alert color="informative" iconName="profile" role="alert">
Message
</Alert>
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the extra test case! 👍🏻

@curdaj curdaj force-pushed the bc/ds-1175-accessibility-for-alert branch from 47059d2 to 28bfdc5 Compare May 10, 2024 10:37
@curdaj curdaj force-pushed the bc/ds-1175-accessibility-for-alert branch from 28bfdc5 to a1e56f5 Compare May 10, 2024 10:46
@curdaj curdaj merged commit 80cae41 into integration/release-v2 May 10, 2024
27 checks passed
@curdaj curdaj deleted the bc/ds-1175-accessibility-for-alert branch May 10, 2024 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants