Skip to content
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 OutlinedInput for correct label handling in SelectArrayInput #8465

Closed
wants to merge 1 commit into from

Conversation

sebastianbuechler
Copy link
Contributor

@sebastianbuechler sebastianbuechler commented Dec 4, 2022

Currently, the label of the SelectArrayInput is not handled correctly when pushed upwards:
image
I used the example code from the react-admin docs: https://marmelab.com/react-admin/SelectArrayInput.html#usage

According to this SO answer this can be fixed directly by MUI: https://mui.com/material-ui/react-select/#MultipleSelectCheckmarks.js

However, I could not test it locally as I was not able to include this fix in my project. I believe I don't properly understand the structure of the react-admin repo in order to consume my fix. I tried it with yarn add https://gitpkg.now.sh/sebastianbuechler/react-admin/packages/react-admin, but on startup, my project can't find react-admin.

@sebastianbuechler
Copy link
Contributor Author

It seems that it does not have the desired effect when opening in the StoryBook:
image

Could it be that react-admin applies some different default styling to the Select component of MUI?

@slax57
Copy link
Contributor

slax57 commented Dec 5, 2022

Just to be sure, could you clear your localStorage and see if the issue is still there?
In the past we had reports of themes being broken because of outdated versions in the store.
Thanks

@sebastianbuechler
Copy link
Contributor Author

Just to be sure, could you clear your localStorage and see if the issue is still there? In the past we had reports of themes being broken because of outdated versions in the store. Thanks

I just tested it with a cleared localStorage and cache, but same result.

If I go to this page: https://mui.com/material-ui/react-select/#MultipleSelectCheckmarks.js And remove the line input={<OutlinedInput label="Tag" />} I get the same result:
image

@slax57
Copy link
Contributor

slax57 commented Dec 5, 2022

@sebastianbuechler

I failed to reproduce the issue by using the outlined variant on a <SelectArrayInput>.

Here is the code I used (inside the SelectArrayInput storybook):

export const Variant = () => (
    <AdminContext i18nProvider={i18nProvider}>
        <Create
            resource="users"
            record={{ roles: ['u001', 'u003'] }}
            sx={{ width: 600 }}
        >
            <SimpleForm>
                <SelectArrayInput
                    source="roles"
                    choices={[
                        { id: 'admin', name: 'Admin' },
                        { id: 'u001', name: 'Editor' },
                        { id: 'u002', name: 'Moderator' },
                        { id: 'u003', name: 'Reviewer' },
                    ]}
                    sx={{ width: 300 }}
                    options={{ variant: 'outlined' }}
                />
            </SimpleForm>
        </Create>
    </AdminContext>
);

Could you provide a repro of the issue?

Besides, I'm a bit worried that your fix will only work for outlined variants. What about other variants?

Lastly, if your PR aims at fixing a bug, it should rather target the master branch.

Thanks.

@@ -253,6 +254,7 @@ export const SelectArrayInput = (props: SelectArrayInputProps) => {
<Select
autoWidth
labelId={`${label}-outlined-label`}
input={<OutlinedInput label={label} />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this will only work with the outlined variant. How about other variants?

@@ -22,6 +22,7 @@ import {
} from 'ra-core';
import { InputHelperText } from './InputHelperText';
import { FormControlProps } from '@mui/material/FormControl';
import OutlinedInput from '@mui/material/OutlinedInput';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this import along the other ones from '@mui/material'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do in the next iteration.

@sebastianbuechler
Copy link
Contributor Author

@sebastianbuechler

I failed to reproduce the issue by using the outlined variant on a <SelectArrayInput>.

Here is the code I used (inside the SelectArrayInput storybook):

export const Variant = () => (
    <AdminContext i18nProvider={i18nProvider}>
        <Create
            resource="users"
            record={{ roles: ['u001', 'u003'] }}
            sx={{ width: 600 }}
        >
            <SimpleForm>
                <SelectArrayInput
                    source="roles"
                    choices={[
                        { id: 'admin', name: 'Admin' },
                        { id: 'u001', name: 'Editor' },
                        { id: 'u002', name: 'Moderator' },
                        { id: 'u003', name: 'Reviewer' },
                    ]}
                    sx={{ width: 300 }}
                    options={{ variant: 'outlined' }}
                />
            </SimpleForm>
        </Create>
    </AdminContext>
);

Could you provide a repro of the issue?

Besides, I'm a bit worried that your fix will only work for outlined variants. What about other variants?

Lastly, if your PR aims at fixing a bug, it should rather target the master branch.

Thanks.

Thank you for your reply.

Here's the replication of the issue: https://stackblitz.com/edit/github-rmhseu-mj4jem?file=src/posts/PostList.tsx

I was not able to easily reproduce it on Storybook, but maybe it would be helpful to have the editable code editor for react-admin storybooks: https://storybook.js.org/addons/storybook-addon-code-editor

Also, you're right, my "fix" would break other variants and I was not aware of them, as we only use the outlined variant.

I found a temporary fix by using:

        <SelectArrayInput
          source="roles"
          variant="outlined"
          choices={[
            { id: 'admin', name: 'Admin' },
            { id: 'u001', name: 'Editor' },
            { id: 'u002', name: 'Moderator' },
            { id: 'u003', name: 'Reviewer' },
          ]}
          options={{ input: <OutlinedInput label="Roles" /> }}
        />

Obviously, this is suboptimal, as the label has to be set twice and it's not intuitive to find this solution.

Interestingly, this bug does not happen for SelectArray with the same outlined variant (I also added it in the Stackblitz).

Do you have any idea how to solve this?

@slax57
Copy link
Contributor

slax57 commented Dec 13, 2022

@sebastianbuechler

I was able to reproduce the issue, and I agree there is something wrong on MUI's side.

As stated in their docs, it should choose the correct input variant automatically:

It extends the text field components sub-components, either the OutlinedInput, Input, or FilledInput, depending on the variant selected.

But in their examples with multiple values, they needed to add input={<OutlinedInput label="Name" />} manually...

IMO this is rather something they should fix on their side, so I'd recommend opening an issue on MUI's repo instead.

If you are willing to provide a workaround in the meantime for RA, here is what I got so far.

In SelectArrayInput.tsx, adding the following lines fixes the label for variant "outlined" only.

                <Select
                    autoWidth
                    labelId={`${label}-outlined-label`}
                    multiple
                    error={
                        !!fetchError || ((isTouched || isSubmitted) && invalid)
                    }
                    renderValue={(selected: any[]) => (
                        <div className={SelectArrayInputClasses.chips}>
                            {selected
                                .map(item =>
                                    (allChoices || []).find(
                                        choice =>
                                            getChoiceValue(choice) === item
                                    )
                                )
                                .filter(item => !!item)
                                .map(item => (
                                    <Chip
                                        key={getChoiceValue(item)}
                                        label={renderMenuItemOption(item)}
                                        className={SelectArrayInputClasses.chip}
                                        size="small"
                                    />
                                ))}
                        </div>
                    )}
                    data-testid="selectArray"
                    size="small"
+                   input={
+                       variant === 'outlined' ? (
+                           <OutlinedInput
+                               label={
+                                   <FieldTitle
+                                       label={label}
+                                       source={source}
+                                       resource={resource}
+                                       isRequired={isRequired}
+                                   />
+                               }
+                           />
+                       ) : undefined
+                   }
                    {...field}
                    {...options}
                    onChange={handleChangeWithCreateSupport}
                    value={field.value || []}
                >
                    {finalChoices.map(renderMenuItem)}
                </Select>

But I'd recommend making sure this kind of workaround is not needed for other variants too, for example by adding new stories to the storybook.

@slax57
Copy link
Contributor

slax57 commented Dec 13, 2022

Related issue: #8498

@sebastianbuechler
Copy link
Contributor Author

@sebastianbuechler

I was able to reproduce the issue, and I agree there is something wrong on MUI's side.

As stated in their docs, it should choose the correct input variant automatically:

It extends the text field components sub-components, either the OutlinedInput, Input, or FilledInput, depending on the variant selected.

But in their examples with multiple values, they needed to add input={<OutlinedInput label="Name" />} manually...

IMO this is rather something they should fix on their side, so I'd recommend opening an issue on MUI's repo instead.

If you are willing to provide a workaround in the meantime for RA, here is what I got so far.

In SelectArrayInput.tsx, adding the following lines fixes the label for variant "outlined" only.

                <Select
                    autoWidth
                    labelId={`${label}-outlined-label`}
                    multiple
                    error={
                        !!fetchError || ((isTouched || isSubmitted) && invalid)
                    }
                    renderValue={(selected: any[]) => (
                        <div className={SelectArrayInputClasses.chips}>
                            {selected
                                .map(item =>
                                    (allChoices || []).find(
                                        choice =>
                                            getChoiceValue(choice) === item
                                    )
                                )
                                .filter(item => !!item)
                                .map(item => (
                                    <Chip
                                        key={getChoiceValue(item)}
                                        label={renderMenuItemOption(item)}
                                        className={SelectArrayInputClasses.chip}
                                        size="small"
                                    />
                                ))}
                        </div>
                    )}
                    data-testid="selectArray"
                    size="small"
+                   input={
+                       variant === 'outlined' ? (
+                           <OutlinedInput
+                               label={
+                                   <FieldTitle
+                                       label={label}
+                                       source={source}
+                                       resource={resource}
+                                       isRequired={isRequired}
+                                   />
+                               }
+                           />
+                       ) : undefined
+                   }
                    {...field}
                    {...options}
                    onChange={handleChangeWithCreateSupport}
                    value={field.value || []}
                >
                    {finalChoices.map(renderMenuItem)}
                </Select>

But I'd recommend making sure this kind of workaround is not needed for other variants too, for example by adding new stories to the storybook.

Opened a ticket here: mui/material-ui#36164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants