fix: Fix keyboard tabbing order in product image zoom view#21353
fix: Fix keyboard tabbing order in product image zoom view#21353
Conversation
spartacus
|
||||||||||||||||||||||||||||
| Project |
spartacus
|
| Branch Review |
feature/CXSPA-12682
|
| Run status |
|
| Run duration | 04m 08s |
| Commit |
|
| Committer | Caine Rotherham |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
101
|
| View all changes introduced in this branch ↗︎ | |
|
|
||
| @include cx-highContrastTheme-dark { | ||
| .cx-image-container .cx-zoom-btn { | ||
| background-color: var(--cx-color-dark); |
There was a problem hiding this comment.
Why remove the background color in this change? Could this be a breaking change for customer implementations using class cx-image-container for their own custom components?
There was a problem hiding this comment.
Both css changes are already encapsulated in the @include forFeature('a11yKeyboardAccessibleZoom'). So the breaking changes have been accounted for.
There was a problem hiding this comment.
But shouldn't the breaking change be accounted for the default context where customer upgrades but feature flag comes in as false, and they keep it as such. So we want to preserve existing styling.
There was a problem hiding this comment.
Yes, it won't be affected if the customer does not enable the a11yKeyboardAccessibleZoom feature flag. We don't really mind too much if we change anything that is already behind this flag since we have to change many things rapidly in an a11y audit. For more complex features, we could consider nested or overriding flags for breaking changes related to breaking changes. But that is a lot of unnecessary tech debt and difficult to maintain for simple changes.
|
|
||
| @include cx-highContrastTheme-light { | ||
| .cx-image-container .cx-zoom-btn .cx-zoom-indicator { | ||
| .cx-image-container .cx-zoom-btn-container .cx-zoom-btn .cx-zoom-indicator { |
There was a problem hiding this comment.
by moving location of #zoomButton outside of the cx-image-container, this include chain and the below one are no longer aligned with the template
There was a problem hiding this comment.
I don't think they are chained because of the spaces between the classes. Or are you suggesting some clean up?
There was a problem hiding this comment.
The way scss interprets line 148 is equivalent to "find a .cx-zoom-indicator that lives inside .cx-zoom-btn inside .cx-zoom-btn-container inside .cx-image-container, and apply these styles to it."
You can even test it, change the color first (you should not see it changed for that theme when you select it in FE) Then remove cx-image-container you should now see the zoom icon change color when the theme is selected
There was a problem hiding this comment.
Ah, I think you might be referring to the styling changes of the zoom button. Sorry, yes this was an intentional change. No styling for this feature was provided by any official ux at the time so something was improvised. As the change was new (within the last year) and still behind the feature flag. I have changed it to be more consistent and easier to maintain.
In regards to breaking changes, we do not mind as its already behind a flag.
2 participants
Closes: https://jira.tools.sap/browse/CXSPA-12682