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

fix(DatagridConfigurable): better detect and handle columns changes #10157

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

quentin-decre
Copy link
Contributor

@quentin-decre quentin-decre commented Aug 26, 2024

Problem

When you change the columns order of a ConfigurableDatagrid, or just if you remove a column and add another one, this is not managed and the column management is broken : aka the displayed columns in the configure list does not control the corresponding columns.

Here is a quick illustration of the issue : https://www.loom.com/share/c5a6afaac36a4111bedf08ece818da17 (in french, sorry)

That's because currently, only a change in the number of columns trigger and update. And even in this case, you may have unexpected results (like the column enabled to be displayed are not the same as before).

Solution

This changes bring :

  • better detection : now, if you change the source of a column, add or remove a column (or both), it triggers the update
  • keeps your previous settings : it keeps the columns you displayed before
  • keeps your order : it keeps the columns displayed in the order you have set
  • enable non omited new columns : if you added new columns, and not omited them, they will be displayed at the end

How To Test

I have tested it multiple times on the following scenarios :

  • change the source of a column
  • add a column and remove another one at the same time
  • change the order of the columns passed to the datagrid
  • add a new non omited columns

I have ran the tests for non regression
image

Also, this PR is live on prod for us now. And this is a very used feature among our users :)

Additional Checks

  • The PR targets master for a bugfix, or next for a feature
  • The PR includes unit tests (if not possible, describe why) : not new feature. tests still pass
  • The PR includes one or several stories (if not possible, describe why) : not a new feature. stories are still relevant
  • The documentation is up to date : no change needed to the documentation

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
It looks promising 🙂
Could you add some unit tests to prove that it works, including in corner cases, and does not break compatibility?
Thanks!

@slax57
Copy link
Contributor

slax57 commented Aug 28, 2024

I have added some example unit tests, showing how you can check what happens when the code is changed, and how to check what's in the store.
Hope this helps!

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