Comment 2 for bug 2057692

Revision history for this message
Galen Charlton (gmc) wrote :

Some initial feedback:

[1] The column visibility checkboxes work for me.
[2] Changing the column order via keyboard or mouse works for me.
[3] The checkboxes for action menu items works for me
[4] ng lint reports issues
[5] npm run test fails (for a reason discussed in the next point)
[6] the grid width component is removed from the grid TS but not the grid HTML, and is not replaced with anything - this also causes a test to fail
[7] How and when the grid config is saved should be discussed. Having the new Save button in the grid columns config modal save the config might be the right call, I think, but that behavior is not consistent with other parts of the grid config. For example, if you edit the actions menu, the change doesn't stick unless you also invoke the save config action.
[8] And actually, whether to automatically save the grid config from the column config modal might be debatable - I don't know the frequency with which users may make temporary changes the grid config that they _don't_ to persist unless explicitly saved.
[9] there are a bunch of whitespace-only changes to the CSS that are not pertinent to the goal of the patch and (IMO) decrease readability by removing indentation.