fix(ui): replace MUI date picker with existing date picker menu#30027
fix(ui): replace MUI date picker with existing date picker menu#30027shah-harshit wants to merge 2 commits into
Conversation
shah-harshit
requested review from
a team,
chirag-madlani and
karanh37
as code owners
shah-harshit
added
UI
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| <Button | ||
| color="secondary" | ||
| iconLeading={CalendarIcon} | ||
| size="md" | ||
| {...buttonProps}> | ||
| {triggerContent} | ||
| {allowClear && value && ( | ||
| <span | ||
| data-testid={clearButtonTestId} | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| setValue(null); | ||
| onClear?.(); |
There was a problem hiding this comment.
💡 Quality: Clear action is an interactive span nested inside a Button
The clear affordance renders a <span role="button" tabIndex={0}> inside the trigger <Button> (a native button). Nesting interactive/clickable content inside a button is invalid HTML and can produce inconsistent focus/keyboard behavior across browsers and screen readers. Consider rendering the clear control as a sibling of the trigger button rather than a descendant, or using a non-button trigger container.
Was this helpful? React with 👍 / 👎
Jest test CoverageUI tests summary
|
shah-harshit
force-pushed
the
ui/replace-mui-date-picker
branch
from
299b0c7 to
6609d05
Compare
| <Button | ||
| color="secondary" | ||
| data-testid="date-range-picker" | ||
| iconTrailing={ | ||
| <DropDownIcon | ||
| className="align-middle" | ||
| height={16} | ||
| width={16} | ||
| /> | ||
| } | ||
| size="sm"> | ||
| <span>{selectedDateRangeLabel}</span> | ||
| {dateRangeKey && ( | ||
| <span | ||
| data-testid="clear-date-picker" |
There was a problem hiding this comment.
💡 Quality: Clear control is an interactive span nested inside a Button
The clear affordance is a <span role="button" tabIndex={0}> nested inside the <Button> that serves as the dropdown trigger, producing nested interactive elements (invalid HTML/ARIA) and a clickable icon with no accessible name. Consider rendering the clear control as a sibling of the trigger button rather than a child, and add an aria-label (e.g. t('label.clear')) to the XClose control.
Add an accessible name; ideally also move it out of the Button to avoid nested interactive elements.:
<span
aria-label={t('label.clear')}
data-testid="clear-date-picker"
role="button"
tabIndex={0}
onClick={...}
onKeyDown={...}>
<XClose className="tw:size-4" />
</span>
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
shah-harshit
force-pushed
the
ui/replace-mui-date-picker
branch
from
6609d05 to
9552691
Compare
|
shah-harshit
changed the title
| @@ -280,7 +287,7 @@ const DimensionalityTab = () => { | |||
| <p className="tw:m-0 tw:text-sm tw:font-medium tw:whitespace-nowrap tw:text-primary"> | |||
There was a problem hiding this comment.
getStartOfDayInMillis and getEndOfDayInMillis, which truncates them again in UTC. For a UTC+05:30 user selecting Mar 4, the picker can pass Mar 3 18:30Z as the local-day start, and this caller then moves it to Mar 3 00:00Z. The dimensionality query can include data from the previous day instead of the selected local range.
| {`${t('label.date')}:`} | ||
| </span> | ||
| <MuiDatePickerMenu | ||
| <DatePickerMenu |
There was a problem hiding this comment.
💡 Bug: TabFilters date label goes stale on external URL changes
DatePickerMenu seeds selectedTimeRange/selectedTimeRangeKey from defaultOptions only at mount (useState initializer). In TabFilters.tsx the defaultDateRange={dateRangeObject} is derived from location.search, but no key prop forces a remount, so when the URL date params change through means other than this picker (e.g. browser back/forward), the trigger label keeps showing the previous range while the fetched data reflects the new URL. The prior MUI picker was controlled via value and did not have this drift. IncidentManager already works around it with a key prop keyed on the range; apply the same pattern to TabFilters (and confirm DimensionalityTab, which uses a constant default, is unaffected).
Force a remount when the URL-derived range changes so the trigger label stays in sync, matching the IncidentManager pattern.:
<DatePickerMenu
showSelectedCustomRange
defaultDateRange={dateRangeObject}
handleDateRangeChange={handleDateRangeChange}
key={`${dateRangeObject.key}-${dateRangeObject.startTs}-${dateRangeObject.endTs}`}
size="small"
/>
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 2 resolved / 5 findingsReplaces MUI date pickers with the standard AntD-backed DatePickerMenu across profiler and incident manager components. Resolve nested interactive elements in the clear control and ensure the date label syncs correctly with external URL changes. 💡 Quality: Clear action is an interactive span nested inside a ButtonThe clear affordance renders a 💡 Quality: Clear control is an interactive span nested inside a ButtonThe clear affordance is a Add an accessible name; ideally also move it out of the Button to avoid nested interactive elements.💡 Bug: TabFilters date label goes stale on external URL changes📄 openmetadata-ui/src/main/resources/ui/src/components/Database/Profiler/DataObservability/TabFilters/TabFilters.tsx:213-218 📄 openmetadata-ui/src/main/resources/ui/src/components/common/DatePickerMenu/DatePickerMenu.component.tsx:124-130
Force a remount when the URL-derived range changes so the trigger label stays in sync, matching the IncidentManager pattern.✅ 2 resolved✅ Bug: Controlled picker without onChange breaks custom range selection
✅ Bug: Date-range trigger button renders blank when no range selected
🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
1 participant
Describe your changes:
Replaced the remaining OSS MUI date picker usages in profiler tab filters, dimensionality, and incident manager with the existing AntD-backed
DatePickerMenu.Incident manager retains preset ranges, places the custom range option last, supports clearing the active filter, preserves the previous placeholder, and keeps the trigger height and clear icon alignment stable. The obsolete
MuiDatePickerMenucomponent, interface, and tests are removed.Type of change:
High-level design:
All three consumers now use the existing AntD-backed
DatePickerMenudirectly. The shared picker accepts optional placeholder and clear callbacks so incident manager can retain its URL-filter behavior without introducing a wrapper or changing core-ui components.Tests:
Use cases covered
Unit tests
Backend integration tests
Ingestion integration tests
Playwright (UI) tests
Verification performed
git diff --check.UI screen recording / screenshots:
Not attached.
Checklist:
Greptile Summary
This PR replaces the MUI date picker usage with the shared DatePickerMenu path. The main changes are:
DatePickerMenu.Confidence Score: 4/5
This is close, but the dimensionality date filter should be fixed before merging.
openmetadata-ui/src/main/resources/ui/src/components/DataQuality/IncidentManager/DimensionalityTab/DimensionalityTab.tsx
Important Files Changed
DatePickerMenu, but custom ranges can be bounded twice.Reviews (4): Last reviewed commit: "fix(ui): use existing date picker menu" | Re-trigger Greptile
Context used (3)