-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add namespace to create environment form #361
Add namespace to create environment form #361
Conversation
I'm not sure this looks the way we want: Side comment: in that screenshot I'm using the MUI TextField component in disabled mode. That seemed like the most straightforward way to add the namespace to the page. However, the colors on disabled text fields in MUI are low contrast, so I think I should probably re-implement the namespace as something else. |
f6d3254
to
5ea6d1f
Compare
@smeragoel when you get a chance could you review this? |
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.
This looks good - see comments though.
it("should render component in create mode with namespace", () => { | ||
const mockOnUpdateName = jest.fn(); | ||
const component = render( | ||
mockTheme( |
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.
For my own understanding: why use mockTheme
instead of <MockTheme>
for that component here?
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.
Good question. Two things:
- I'm not super familiar with MUI or testing MUI. I was just patterning after what's already in this test file.
- The function
mockTheme
is not written as a functional React component, but it could be.
The utility function is so lightweight that I'm thinking it might be better to replace the call to mockTheme() everywhere with:
import { condaStoreTheme } from "../src/theme";
// ...
<ThemeProvider theme={condaStoreTheme}>
<Provider store={store}>
<ComponentUnderTest />
</Provider>
</ThemeProvider>
@gabalafou and I discussed this issue and we decided that we're gonna see how much effort it is to customize the input component to match the designs and then we can decide the way forward |
5ea6d1f
to
cf70874
Compare
This reverts commit 4525307.
Given the conversation in today's planning meeting, I reverted my last commit in favor of using the MUI TextField component without customization |
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.
Looks good - thanks for adding that aria-hidden
property. Can you make sure the playwright tests are okay before merging?
The Playwright test is probably a legit failure. Need to investigate. |
Merged since all checks pass! |
Fixes one half of #307.
Description
This pull request shows the namespace on the form/tab to create a new environment.
Pull request checklist