Repository navigation
fix(MobilePowerSearch): refine touch filter flow - #8749
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
3c3be1b to
8d684a5
Compare
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMobilePowerSearch (@astryxdesign/lab) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| lab/MobilePowerSearch | +6.1KB |
-177B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/lab | 4.6KB | 1.9KB | 1.9KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/lab | 11.1KB | 11.2KB | +54B |
Accessibility Audit
Status: No accessibility violations detected.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
8d684a5 to
9e83893
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval of this pull request needs an engineering owner's spec decision first: the Lab touch flow's new direction (a retained sheet stack, removal from the Filters rows, one-tap empty filters, a Clear all that keeps read-only filters, inline list and calendar editors, and timezoneID-based dates) has no current spec for the Lab component, and component:PowerSearch DEC-3 currently describes this flow and rejects two of those choices. An engineering owner settles it by amending that spec or recording governing spec for the Lab prototype and commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Separately, please split this 2,045-line change and fix:
- Date-only filters now show a time of day on tokens and Filters rows (
MobilePowerSearch.tsx:679). - Typing in Search filters after scrolling the list keeps only the first letter (
MobilePowerSearch.tsx:2160). - A filter such as Status is empty on a field with other operators is removed by one tap on its row and exposes no checked state (
MobilePowerSearch.tsx:1998).
[Automated review]
| ) { | ||
| return formatDateDay(filter.value.unixSeconds, locale); | ||
| } | ||
| return formatFilterValue( |
There was a problem hiding this comment.
Date-only values now go through formatFilterValue, whose formatDateAbsolute always adds hour and minute, so a date-only filter's token and Filters row read like Mar 9, 2024, 7:00 PM (and a day picked in the calendar reads 12:00 AM). Consider formatting date_absolute values whose operator is date-only as a day in timezoneID, keeping the time only for isDateOnly: false.
| const fieldLabel = | ||
| config.getField(row.filter.field)?.label ?? | ||
| row.filter.field; | ||
| const isImmediateToggle = operator?.value.type === 'empty'; |
There was a problem hiding this comment.
This makes every empty-operator filter a one-tap removal, but handleFieldSelect only treats single-operator fields that way. For a field like Status with is / is not / is empty, a Status is empty row can no longer be edited and a tap removes it. The checked indicator is aria-hidden on a plain button, so assistive tech gets no checked state either. Suggest limiting this to single-operator fields and exposing the state (a checkbox with aria-checked) or naming the action.
| <CheckboxIndicator state="checked" /> | ||
| ) : ( | ||
| <IconButton | ||
| label={`Remove ${fieldLabel} filter`} |
There was a problem hiding this comment.
Two filters on the same field get identical names (Remove Author filter twice), so screen-reader and voice users can't tell which one a button removes. Naming it from the row's full description (for example Remove Author is Ada) would disambiguate.
| }} | ||
| <div | ||
| {...scrollViewportProps} | ||
| onScroll={blurFieldPickerSearch} |
There was a problem hiding this comment.
Blurring on every scroll also fires when typing narrows the list: the browser clamps scrollTop, a scroll event follows, and the search loses focus mid-word. In Chromium, after scrolling Add filter and typing pri into Search filters, only p remains. Dismissing the keyboard only on a user drag (the onTouchMove path) avoids it.
| [selectedItems], | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
results starts as [] and is filled after a setTimeout, so the first render shows No results found before the search source answers. A pending state until the first search settles would avoid the flash; since this effect calls the caller's search source and sets state, a comment naming that external system would also help.
| icon={<Icon icon="search" size="lg" color="secondary" />} | ||
| title={ | ||
| allowsCreate && query.trim() === '' | ||
| ? `Add ${label === 'ID' ? 'an ID' : 'a value'}` |
There was a problem hiding this comment.
This picks Add an ID only when the caller's field label is exactly ID and builds the sentence from English literals, so other labels and locales get Add a value. One translated message with the field label as a placeholder would cover every field.
9e83893 to
8a31f7e
Compare
8a31f7e to
f667979
Compare
f667979 to
b449534
Compare
153fc57 to
c73efd0
Compare
Summary
Test plan
pnpm exec vitest run packages/lab/src/MobilePowerSearch/MobilePowerSearch.test.tsx(115 tests)MobilePowerSearch.tsxandMobilePowerSearch.test.tsxpackages/lab/src/Comments/icons.test.tsx; no changed MobilePowerSearch file reports a type error