hyva-themes / magento2-hyva-admin

This module aims to make creating grids and forms in the Magento 2 adminhtml area joyful and fast.
https://hyva-themes.github.io/magento2-hyva-admin/
BSD 3-Clause "New" or "Revised" License
168 stars 39 forks source link

Add initiallyHidden columnlist option #22

Closed paugnu closed 3 years ago

paugnu commented 3 years ago

Hi @Vinai,

I've added the new column option. As you described

If you have any recommendations, comments, modifications to perform... please let me know.

Vinai commented 3 years ago

Regarding the failing unit tests - I updated the CI script, so if you rebase on main that should take care of that problem. Sorry for the inconvenience.

paugnu commented 3 years ago

I added the secrets to my repo and seemed to work :)

paugnu commented 3 years ago

I added the secrets before reading your comment about rebasing. I've just rebased (but I'm not sure I did it correctly :S).

Vinai commented 3 years ago

Thanks for the update. I was just about to merge the PR, when I noticed the sortable attribute for columns. This is also a boolean attribute, but it doesn't use the is prefix. In the current state a column declaration could look like:

<column name="foo" sortable="false" isInitiallyHidden="true"/>

This is not consistent.

I'm sorry I made the mistake suggesting isInitiallyHidden, but now I think it should be initiallyHidden instead. In the class \Hyva\Admin\ViewModel\HyvaGrid\ColumnDefinition::isInitiallyHidden still is consistent with how the other boolean attributes work.

I can either merge the PR and make the change myself, but I want to give you the choice to do it, too - whatever way you prefer.

paugnu commented 3 years ago

Hi @Vinai, of course I'll do it :). I like to do things as consistent as possible :) (I'm pretty bad at it anyway!!).

Vinai commented 3 years ago

It's surprising to me how hard it is to keep things consistent :)

paugnu commented 3 years ago

Hi @Vinai,

It was good we tried to be consistent. While performing some tests before pushing, I run into an annoying error:

  1. Be sure you don't have the xx-grid-hiddenCols in your session storage
  2. Visit the grid for the first time
  3. Hide/Display columns »»» Expected result: the xx-grid-hiddenCols session variable should be updated »»» Actual result: xx-grid-hiddenCols is not updated

I've tried to find the reason for this. Basically, the error is caused because of the modification of initializing hiddenCols and then retrieving it from the getHiddenCols function (when no session variable has been set).

What is weird, is that if I harcode the array in the getHiddenCols function, it works fine.

This could be easily fixed by removing the @watch action from init and simply calling this.storeHiddenColumns(); at the end of the toggleColumn function (tested and worked). But I'm not sure you want this to be done this way.

Is there any other way we can make @watch work the first time we visit the grid?

Vinai commented 3 years ago

I'll check out your PR branch to have a look, too.

Vinai commented 3 years ago

I think your suggested solution is good. Removing this.$watch('hiddenCols', this.storeHiddenColumns.bind(this)); from initState and calling this.storeHiddenColumns(); from toggleColumn is good.

Note to myself: the $watch still has to be used for the selected rows, as there the checkboxes are bound directly to the array and no toggle function is used.

Vinai commented 3 years ago

Awesome, thank you! 🤗

paugnu commented 3 years ago

Thanks for your patience and help, Vinai :)