-
Notifications
You must be signed in to change notification settings - Fork 42
Upgrade MUI version #2786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Upgrade MUI version #2786
Conversation
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
| const theFuture = dayjs().add(1, 'day').format(dateTimeFormatISO); | ||
|
|
||
| await user.type(expiryDateInput, thePast); | ||
| fireEvent.change(expiryDateInput, { target: { value: thePast } }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fayi-da apparently that’s how we fill date fields in unit tests now 🙂
The await user.type call leaves the field in an Invalid Date state, which messes up the tests.
Kind of funny, because when I was writing my first UI tests, I remember fireEvent.change not working quite right
|
|
||
| await waitFor(() => { | ||
| expect(screen.queryByText('Expiration must be in the future')).toBeDefined(); | ||
| expect(screen.queryByText('Expiration must be in the future')).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fayi-da for some reason, toBeDefined was giving me both false positives and false negatives (it seems like screen.queryByText returns a DOM-like Null instead of a standard JS null). Switching to toBeInTheDocument fixed the existing test errors and revealed some new ones (e.g. apps/sv/frontend/src/components/forms/SetAmuletConfigRulesForm.tsx). I’d propose we use toBeInTheDocument from now on
| field.handleChange(e.target.value as string) | ||
| } | ||
| onBlur={field.handleBlur} | ||
| placeholder="Select an action" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MUI’s <Select /> doesn’t support a placeholder prop, so it doesn’t do anything (see PR #1626), and in v7 it actually throws an error. If we want to match the Figma designs, we’ll need to hack in a placeholder manually.
| > | ||
| <Grid container spacing={1}> | ||
| <Grid xs={3}> | ||
| <Grid size={{ xs: 3 }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
Signed-off-by: Paweł Perek <[email protected]>
| maxDateTime={effectivity} | ||
| readOnly={false} | ||
| onChange={d => handleExpirationDateChange(d)} | ||
| enableAccessibleFieldDOMStructure={false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context: In MUI v7, the DatePickers introduced a new “accessible” structure. Instead of a simple <input />, the picker is now composed of multiple <div> elements with tab-selectable slots (year, month, day, etc.), plus a read-only <input /> managed by those <div>s.
While this approach is great for UX and accessibility, it’s absolutely horrible for integration testing. I’ve tried several methods to programmatically fill these pickers, including:
sendKeysto the read-only<input />(current approach)sendKeysto the parent<div>that manages the DatePickersendKeysdirectly to individual slot<div>s (fails due to remounting)sendKeysto slot<div>s while searching for the next slot after each inputjs.executeScriptto focus/click the managing<div>and thensendKeys(works in the browser but not in tests)js.executeScriptto set the value directly in the script
...and several other variations, but none of them worked reliably.
Given that, I propose we keep using the pre-v7 non-accessible structure for now. Unfortunately it’s soft-deprecated and while it doesn’t raise warnings yet, it's planned for removal in v9, so I hope either it will be fixed until then or we will figure our some trick to fill it
Fixes #2733
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines