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

<DatagridConfigurable omit=... does nothing #8817

Closed
Dreamsorcerer opened this issue Apr 7, 2023 · 17 comments · Fixed by #8851
Closed

<DatagridConfigurable omit=... does nothing #8817

Dreamsorcerer opened this issue Apr 7, 2023 · 17 comments · Fixed by #8851
Assignees

Comments

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Apr 7, 2023

I've reread the 2 lines of documentation and example more than a dozen times to see if I'm missing some detail, but the omit prop just doesn't seem to do anything.

What you were expecting:
The columns listed in omit to be hidden by default and the SelectColumnsButton to show them as hidden.

What happened instead:
Nothing. It still renders the columns exactly the same as not using the omit prop.

However, I can see the omit value appears in local storage, which suggests I correctly used the omit prop.

Steps to reproduce:
I don't have a simple reproducer, as the project builds everything from a JSON schema, but the actual code to create the component is:

        <List actions={<ListActions />} filters={createInputs(resource, name, "view", permissions)}>
            <DatagridConfigurable omit={resource["list_omit"]} rowClick="show" bulkActionButtons={<BulkActionButtons />}>
                {createFields(resource, name, permissions)}
                <WithRecord render={(record) => hasPermission(`${name}.edit`, permissions, record) && <EditButton />} />
            </DatagridConfigurable>
        </List>

With a console.log() I can see that resource["list_omit"] is set to ["value", "num"] in my test example.
I can also see in local storage, there is a new setting with the key RaStore.preferences.simple.datagrid.omit and value ["value","num"]. So, it appears to be set correctly. However, both of those columns are still displayed by default.

Other information:
Documentation: https://marmelab.com/react-admin/Datagrid.html#configurable

image

Environment

  • React-admin version: 4.3.0 (retested on 4.9.1)
  • React version: 18.2.0
  • Browser: Firefox
@slax57
Copy link
Contributor

slax57 commented Apr 7, 2023

Not sure this is really a bug but rather a misunderstanding: the omit prop is only used when you don't already have a configuration saved in the store. To see it in action, you need to clear your localStorage to simulate a user accessing the App for the first time.
Can you give it a try?

@Dreamsorcerer
Copy link
Contributor Author

Sorry, I forgot to include that in the steps. I deleted everything from localStorage when testing several times (I think it automatically deleted on logout as well).

@Dreamsorcerer
Copy link
Contributor Author

image

image

@Dreamsorcerer
Copy link
Contributor Author

When I toggle the columns as a user, it also stores the current state in RaStore.preferences.simple.datagrid.columns with indexes. So, I don't really understand why it's storing the omit value in the first place, as it appears to have nothing to do with the user.

@slax57
Copy link
Contributor

slax57 commented Apr 11, 2023

I'm sorry but I don't reproduce the issue.
I've changed the value of omit in this stackblitz and it works as expected.

Now, I agree that the documentation on this feature is still quite poor, hence I'll label this issue as documentation so that we try to improve it.

To answer your other questions, the value of omit is stored in the Store because we need to share its value across several components, and we did not want to create a new React Context because it would need to be at a very high level in the React Tree, hence triggering too many re-renders when the value changes and hurting performances.

This prop is useful because it acts as a kind of default value when you clear the preferences for the ConfigurableDatagrid using the InspectorButton (see screenshot below).

Capture d’écran du 2023-04-11 09-39-01

@Dreamsorcerer
Copy link
Contributor Author

Dreamsorcerer commented Apr 11, 2023

Not sure how easy it is for you to run Python, but this is the actual code:
https://github.com/aio-libs/aiohttp-admin/blob/configurable-columns/admin-js/src/App.js#L210-L218

It should be possible to run with something like:

git clone https://github.com/aio-libs/aiohttp-admin.git
git checkout configurable-columns
cd aiohttp-admin/
pip3 install -r requirements.txt  # Or maybe pip (without the 3)
cd admin-js/
yarn install
yarn build
cd ../examples/
PYTHONPATH='..' python3 permissions.py  # Or maybe python (without the 3).

Then go to http://localhost:8080/admin
(login is 'admin' with any password)

I'll try and include the InspectorButton (hadn't got that far through the documentation yet) and see what happens when that is used.

@Dreamsorcerer
Copy link
Contributor Author

Yep, same result with InspectorButton, when clicking the reset button it resets to all columns showing, despite the setting in the storage still appearing as ["value","num"].

I can't run the example you linked as it seems to need several GBs of RAM that I don't have spare currently, so my system crashes before it finishes loading.

@slax57
Copy link
Contributor

slax57 commented Apr 12, 2023

You need to understand that we receive multiple issues each day, so it would be impossible for us to take the time needed to run and debug custom projects for each issue.
That's why we require our users to reproduce them in the provided codesandbox or stackblitz.

If there is really an issue with the omit prop on this component, il should definitely be possible to reproduce it inside a sandbox.

Since all of the tests I have performed so far do confirm the component is working as expected, I won't investigate any further on this issue unless you manage to reproduce it inside our sandbox.

@Dreamsorcerer
Copy link
Contributor Author

Dreamsorcerer commented Apr 13, 2023

Yep, I know. I don't get paid to maintain the dozens of projects I'm involved in, so I probably have even less time. :P

Seems that I was just on the verge of RAM usage, managed to free up a little more and got it running. I've copied the code in and then hacked the code a little to use a fake API and skip some checks. That seems to have reproduced the issue again:
https://stackblitz.com/edit/github-9jgv9m-rdxcen?file=src/index.tsx
(I've just done it all in the index.tsx file, but didn't bother to delete all the other files, so just ignore them).
In case I've not done that correctly, I'm also attaching the file, so you can just paste it in if needed.
index.tsx

@slax57
Copy link
Contributor

slax57 commented Apr 17, 2023

I'm sorry but your sandbox looks nothing like the simple example anymore, so it's impossible to see what you have changed in it. Besides it throws many errors and warnings at runtime, e.g. Each child in a list should have a unique "key" prop which is a heavy code smell.

What you are asking us here is basically to debug your code, instead of providing evidence there is a bug in the library.

Please note that having a member of our team help debug your application is possible though, by purchasing Professional Services.

As such, I'll close this issue, and invite you to either resort to Professional Services, or provide a clear evidence that you found a bug coming from react-admin.

@slax57 slax57 closed this as completed Apr 17, 2023
@Dreamsorcerer
Copy link
Contributor Author

OK, finally had time to break this down further. The problem appears to be:

<WithRecord label={state["props"]["label"] || field} render={
    (record) => hasPermission(`${name}.${field}.view`, permissions, record) ? c : <VisibilityOffIcon />
} />

It works correctly if I add source={field} to match the inner component (c):

<WithRecord source={field} label={state["props"]["label"] || field} render={
    (record) => hasPermission(`${name}.${field}.view`, permissions, record) ? c : <VisibilityOffIcon />
} />

I don't see any mention of source in the documentation or examples for WithRecord:
https://marmelab.com/react-admin/WithRecord.html

So, I'm not sure how I was supposed to know that was required (but, maybe I missed something elsewhere in the documentation?).

@fzaninotto
Copy link
Member

That's because the Datagrid has to inspect its children to determine their label. It is explained in the first paragraph of the Datagrid usage documentation.

https://marmelab.com/react-admin/Datagrid.html#usage

The same goes for ConfigurableDatagrid.

If you use WithRecord with no label, Datagrid has no way of knowing which column is which. Now I agree that when people use WithRecord they may forget it, and we can remind them in the WithRecord documentation. Reopening the issue to fix the documentation.

@Dreamsorcerer
Copy link
Contributor Author

That's because the Datagrid has to inspect its children to determine their label. It is explained in the first paragraph of the Datagrid usage documentation.

Hmm, not quite. It says that it uses source if label is not defined. If you refer back to the previous comment, label was already defined. If you look back to the screenshots earlier, you'll also see that the labels were appearing correctly in the Datagrid.

So, it seems that it requires source despite label being defined...

@fzaninotto
Copy link
Member

fzaninotto commented Apr 24, 2023

Right, I'm reopening the issue to investigate further.

@fzaninotto fzaninotto reopened this Apr 24, 2023
@fzaninotto
Copy link
Member

The documentation for the Configurable datagrid clearly says:

By default, <DatagridConfigurable> renders all child fields. But you can also omit some of them by passing an omit prop containing an array of field sources

React-admin needs to match these sources with the field sourcs. If you define a custom field without source, there is no way it can work.

In other words, if you want to use omit, you must set the source on each field.

So I'm closing this issue as we won't fix it.

@Dreamsorcerer
Copy link
Contributor Author

Right, but why not include source in the WithRecord documentation/examples, so it's more likely to work first time for users? There's no indication it's even a valid prop, I just made a guess.

I guess this also means that omit as a feature is limited to only columns that map to a field? e.g. There's no way to omit the edit button column in my original example.

@slax57
Copy link
Contributor

slax57 commented Apr 25, 2023

@Dreamsorcerer you can wrap any custom component into a component that accepts a source prop, even though it does not actually use it.

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

Successfully merging a pull request may close this issue.

3 participants