Skip to content

fix: (CXSPA-9010) Previous field error message read. #20071

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Mar 31, 2025
Merged

fix: (CXSPA-9010) Previous field error message read. #20071

merged 13 commits into from
Mar 31, 2025

Conversation

StanislavSukhanov
Copy link
Contributor

@StanislavSukhanov StanislavSukhanov requested review from a team as code owners March 12, 2025 10:14
@github-actions github-actions bot marked this pull request as draft March 12, 2025 10:14
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 12, 2025 10:25
@StanislavSukhanov StanislavSukhanov self-assigned this Mar 12, 2025
@cypress Cypress
Copy link

cypress bot commented Mar 12, 2025 •
edited
Loading

spartacus    Run #47475

Run Properties:  status check passed Passed #47475  •  git commit b8574a2ad8 ℹ️: Merge e525d251c6881e2d0209ec43a725aadccf296cec into 1bcef5effcec63a6759cd9b06ac0...
Project spartacus
Branch Review feature/CXSPA-9010-previous-error-message-is-read-to-user
Run status status check passed Passed #47475
Run duration 04m 38s
Commit git commit b8574a2ad8 ℹ️: Merge e525d251c6881e2d0209ec43a725aadccf296cec into 1bcef5effcec63a6759cd9b06ac0...
Committer Stanislav Sukhanov
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 118
View all changes introduced in this branch ↗︎

Pio-Bar
Pio-Bar previously approved these changes Mar 12, 2025
View reviewed changes
@StanislavSukhanov StanislavSukhanov force-pushed the feature/CXSPA-9010-previous-error-message-is-read-to-user branch from e65e7fe to 9aa51c5 Compare March 25, 2025 10:08
@github-actions github-actions bot marked this pull request as draft March 25, 2025 10:09
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 25, 2025 10:10
Zeyber
Zeyber requested changes Mar 25, 2025
View reviewed changes
projects/storefrontlib/shared/components/form/form-errors/form-errors.component.ts Outdated
@@ -35,13 +38,24 @@ import { map, startWith } from 'rxjs/operators';
selector: 'cx-form-errors',
templateUrl: './form-errors.component.html',
changeDetection: ChangeDetectionStrategy.OnPush,
host: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could use @HostBindings for these attributes like you did for attr.role? I think it could be easier to understand and modify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor Author

@StanislavSukhanov StanislavSukhanov Mar 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For me, personally, using host in the component decorator is more readable.
https://angular.dev/guide/components/host-elements
here we have decorators listed as 'alternatively'

Copy link
Contributor

Choose a reason for hiding this comment

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

This is completely reasonable point. I was just confused seeing both. I think its better to be consistent with whichever was there first just in my opinion.

@github-actions github-actions bot marked this pull request as draft March 26, 2025 12:46
@StanislavSukhanov StanislavSukhanov force-pushed the feature/CXSPA-9010-previous-error-message-is-read-to-user branch from 64fafc9 to 079d4b4 Compare March 27, 2025 09:06
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 27, 2025 09:19
Zeyber
Zeyber requested changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@Zeyber Zeyber left a comment

Choose a reason for hiding this comment

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

We should also unit test the form errors component since for the aria-live attribute and the ngDoCheck as the logic become more complex.

projects/storefrontlib/shared/components/form/form-errors/form-errors.component.ts Outdated
? null
: 'alert';

@HostBinding('attr.aria-live') get ariaLiveValue() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive my ignorance on this one. Is the get ariaLiveValue() a real method? How is it working if so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not at all, the reason it was set a a method was because it wasn't working in conjunction with a signal I've previously used. let me correct it

projects/storefrontlib/shared/components/form/form-errors/form-errors.component.ts Outdated
if (!this.featureConfigService.isEnabled(
'a11yImprovedErrorMessage'
)) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this override any preset aria-live value to null? Or its ok because we don't set it to anything?

Copy link
Contributor Author

@StanislavSukhanov StanislavSukhanov Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is interesting thing. Let me check it out.
Initial intent was to ignore the aria-live, because if we do not have 'a11yImprovedErrorMessage' enabled, the role of the host is being set to alert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it. now it's a bulletproof and doesnt ignore the aria-live attribute if it's being set from outside

@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build.
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft March 27, 2025 09:23
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 27, 2025 12:09
@github-actions github-actions bot marked this pull request as draft March 27, 2025 12:09
Zeyber
Zeyber requested changes Mar 28, 2025
View reviewed changes
Copy link
Contributor

@Zeyber Zeyber left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice work! Lets just add the unit tests for making sure aria-live is correctly set now on the form errors component.

@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 28, 2025 10:04
@github-actions github-actions bot marked this pull request as draft March 28, 2025 12:35
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 28, 2025 12:39
@github-actions github-actions bot marked this pull request as draft March 28, 2025 12:39
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 28, 2025 12:41
@github-actions GitHub Actions
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build.
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft March 31, 2025 10:08
@StanislavSukhanov StanislavSukhanov marked this pull request as ready for review March 31, 2025 10:13
@StanislavSukhanov StanislavSukhanov merged commit cf91085 into develop Mar 31, 2025
28 checks passed
@StanislavSukhanov StanislavSukhanov deleted the feature/CXSPA-9010-previous-error-message-is-read-to-user branch March 31, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zeyber Zeyber Zeyber approved these changes

@Pio-Bar Pio-Bar Pio-Bar left review comments

Assignees

@StanislavSukhanov StanislavSukhanov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@StanislavSukhanov @Zeyber @Pio-Bar