-
Notifications
You must be signed in to change notification settings - Fork 386
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(a11y): add a11y selected label improvements to checkout Address/PaymentMethod components #19381
base: develop
Are you sure you want to change the base?
Conversation
…DeliveryAddress/CheckoutPaymentMethod components
spartacus Run #45252
Run Properties:
|
Project |
spartacus
|
Run status |
Passed #45252
|
Run duration | 11m 58s |
Commit |
cbebb7ad03 ℹ️: Merge e3d77e3d59a8a79a06cace548bf8a2ac4484fdc8 into ec946e913298491b43312c165626...
|
Committer | Uros Lates |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
4
|
Pending |
2
|
Skipped |
0
|
Passing |
125
|
Upgrade your plan to view test results. |
@@ -329,4 +335,12 @@ export class CheckoutDeliveryAddressComponent implements OnInit { | |||
protected shouldUseAddressSavedInCart(): boolean { | |||
return !!this.checkoutConfigService?.shouldUseAddressSavedInCart(); | |||
} | |||
|
|||
private getCardRole(isCardSelected: boolean): 'button' | 'region' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the feature flags here! Lets make this method protected
instead of private
so that lib users can still access it if they have a need to see/modify it without cloning the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets also add a unit test assertion to check that the role
is set correctly since there is some logic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Addressed both of these in latest commit!
...e-libs/checkout/base/components/checkout-payment-method/checkout-payment-method.component.ts
Show resolved
Hide resolved
…method for checkout-delivery-address.component.ts
…method for checkout-payment-method.component.ts
).role).toEqual('region'); | ||
}); | ||
|
||
it('should be set to "region" when feature flag is disabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't really need to test for the feature flags because they will be removed anyway next major and we can always assume they are active for unit tests. The main thing to test here was that isSelected
sets the role correctly. Is it possible to remove any reference to feature flags here?
@@ -500,4 +562,11 @@ describe('CheckoutDeliveryAddressComponent', () => { | |||
expect(getSpinner()).toBeFalsy(); | |||
}); | |||
}); | |||
|
|||
// describe('getCardRole()', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over?
expect(component['getCardRole']).toHaveBeenCalledWith(true); | ||
}); | ||
|
||
it('should be set to "region" when feature flag is disabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as below about testing feature flags.
it('should be set to "region" for selected addresses when feature flag is enabled', () => { | ||
spyOn(featureConfig, 'isEnabled').and.returnValue(true); | ||
expect(component.getCardContent( | ||
// isSelected = true! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over reference note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Addressed all test related comments in latest commit.
No description provided.