feat(a11y): Redesign searchbox to include label #19222
Conversation
spartacus
|
||||||||||||||||||||||||||||||
| Project |
spartacus
|
| Branch Review |
feature/CXSPA-7990
|
| Run status |
|
| Run duration | 13m 17s |
| Commit |
|
| Committer | sdrozdsap |
| 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. | |
| View all changes introduced in this branch ↗︎ | |
sdrozdsap
force-pushed
the
feature/CXSPA-7990
branch
from
6c5b71e to
6e0f3db
Compare
| // as this will no longer emit a focus event to the controller logic | ||
| width: 0; | ||
| padding: 0; | ||
| // cxFeat_a11ySearchboxLabel class is only applied if a11ySearchboxLabel flag is true |
There was a problem hiding this comment.
Thinking about this one. Arguments against this approach could be:
cxFeat_a11ySearchboxLabelis being publicly exposed. Users may decide to override this when enabling the feature flag as they will not read these comments.- This approach is unusual and a bit difficult to consider all possibilities. It will also be a bit difficult for anyone who needs to clean it up as its a non-standard approach. Could you please elaborate more on the issues it resolves?
There was a problem hiding this comment.
I agree with you: the points that you mention are solid arguments against this, but at the moment I didn't have any other thought in my mind. The main reason why I dived in this workaround, is because this component applies styles to the <body> element, which is impossible to do with the forFeature mixin. Other reason, is that there are big styling changes, and following the standard procedure with the forFeature mixin didn't seem doable. Its assumed that with this mixin we would be always able to override the styles if feature flag is enabled, but in this case it would be very nice to have ability to apply styles for when this flag is NOT enabled. Otherwise I need a way to "reset" a lot of styles, that would be applied if the feature flag is disabled(you can take a look at projects/storefrontstyles/scss/components/product/search/_searchbox.scss starting line 141 - there are all styles which I don't need). One more point worth mentioning is that nested forFeature don't work, but I would end up having them if I would go with this approach.
Considering your points and my thought above, I came up with an idea, but probably it's not a good one: we can create a new component, copying all the functionality from the old one, but having all new styles and a11y adjustments. Then we can decide depending on whether the feature flag is enabled or not, which component to use.
Please let me know what you think
There was a problem hiding this comment.
Thank you for clarifying the reasons you took this approach. I think that my concerns around clean up and confusion are not weighted enough to justify rethinking another approach considering the complexity of working something out and the scenario of any major issues coming out of it are very low IMHO. Thank you for making these considerations :)
sdrozdsap
force-pushed
the
feature/CXSPA-7990
branch
from
6e0f3db to
2170394
Compare
sdrozdsap
force-pushed
the
feature/CXSPA-7990
branch
from
2170394 to
ffa1eed
Compare
Merge Checks Failed |
sdrozdsap
force-pushed
the
feature/CXSPA-7990
branch
from
ffa1eed to
c544f58
Compare
Merge Checks Failed |
sdrozdsap
force-pushed
the
feature/CXSPA-7990
branch
from
c544f58 to
d04a1f1
Compare
Ticket: https://jira.tools.sap/browse/CXSPA-7990