fix(mwpw-177207): adds visible Sort by: label to sort dropdown#451
Open
cmiqueo wants to merge 1 commit into
Open
fix(mwpw-177207): adds visible Sort by: label to sort dropdown#451cmiqueo wants to merge 1 commit into
cmiqueo wants to merge 1 commit into
Conversation
shkhan91
approved these changes
May 11, 2026
raissanjay
approved these changes
May 13, 2026
Collaborator
There was a problem hiding this comment.
Tested locally
- Built the PR branch (
NODE_ENV=production npx webpack) — emitteddist/main.js(890 KB) anddist/app.css(227 KB), no warnings - Confirmed bundle contains the new strings:
consonant-Select-label,sort-by-Select,Sort by: - Served the built dist via WebStorm's built-in server (
localhost:63342/caas/), compared before/after on the same test page - DOM checks on the rendered after state:
[data-testid="consonant-Select-label"]element renders- Label text content =
"Sort by:" - Label
htmlFor="consonant-Select-btn" - Button
id="consonant-Select-btn"(matcheshtmlFor) - Label has no
aria-labelattribute - Wrapper carries both
consonant-Selectandsort-by-Selectclasses - Wrapper computed
display: flex, label computedfont-size: 13px - Gap between label end and button start measured at 0 px (baseline alignment works)
- Dropdown still functional: click opens popup with all 6 sort options (Random, Featured, Title A-Z, Title Z-A, Date Oldest, Date Newest),
aria-expandedtoggles to"true" - New test file syntax-parses without errors (
node --check) - Top-filter panel layout (
filterPanel.type: 'top'): switched test page config, verified the layout reflows correctly..consonant-TopFiltersrenders, sort widget sits at far right per the newjustify-content: flex-end(replacingspace-between), wrapper sized to fit just label + button (137 px width), no orphan whitespace. The CSS changes inside@media @consonant-desktop-upapply cleanly. - Narrow viewport (375 px): constrained body to 375 px via injected style and reloaded —
Sort by:label and button remain adjacent on a single line with baseline alignment intact, no wrap or overlap. (Note: thepanel.lesswidth-removal changes are inside@media @consonant-desktop-up— theselect.less.sort-by-Selectrule applies at all widths.) - i18n override path: injected
i18n: { sortBy: 'Trier par : (custom)' }into the test page's collection config; label rendered the custom string verbatim. ConfirmsgetConfig('collection', 'i18n.sortBy')reads through the override correctly. Authors can swap the label text without touching React code. - RTL layout (
<html dir="rtl" lang="ar">): the whole page mirrors as expected — filter panel moves to the right, cards reorder, and the sort widget moves to the visual top-left (mirror of LTR top-right). Inside the widget,display: flexflips the label/button visual order soSort by:appears on the visual right and the dropdown on the visual left, reading naturally in RTL order. Colon position is correctly handled by the Unicode bidi algorithm for English text in an RTL context (would render properly in actual Arabic).
Approving. Verified items look clean across desktop, top-filter layout, narrow viewport, i18n override, and RTL. The only thing I'd nudge before merge is the dead commented-out line in panel.less (inline comment above).
| & > div:first-child > button { | ||
| padding-left: 12px; | ||
| padding-right: 12px; | ||
| // padding-left: 12px; |
Collaborator
There was a problem hiding this comment.
Worth deleting outright rather than leaving it comented out. Lints will skip it but the next person edting this rule will wonder whether 12px was meant tocome back. If the intent is to preserve the originalvalue for reference, a one-line comment above explaiing why (e.g. "intentionally no left padding, label its to the left") would carry more signal than the lteral old line.
| const sortPopLabel = screen.getByTestId('consonant-Select-label'); | ||
|
|
||
| expect(sortPopLabel).toHaveTextContent('Sort by:'); | ||
| expect(sortPopLabel).not.toHaveAttribute('aria-label'); |
Collaborator
There was a problem hiding this comment.
The not.toHaveAttribute('aria-label') assertion is the right anti-regression check. Without it, a future patch could easily reintroduce an aria-label thinking it adds value, and screen readers would then announce the same control twice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Adds a visible text label before the sort dropdown button in the Consonant top-filter panel, replacing the implicit-only aria-label with an explicit paired element.
L10n:
It can be localized by using the following new key in the mappings file
sortBy: "Sort by:"Resolves:
https://jira.corp.adobe.com/browse/MWPW-177207
Before:
After:
