Add column list options #4170
Add column list options #4170
Conversation
trickreich
added
Bug
| color: $fontColor; | ||
| cursor: pointer; | ||
| font-size: 12px; | ||
| margin-bottom: 13px; |
There was a problem hiding this comment.
We usually rounded the margin values to even values, didn't we?
There was a problem hiding this comment.
Never heard of that..
| cursor: pointer; | ||
| font-size: 12px; | ||
| margin-bottom: 13px; | ||
| position: relative; |
There was a problem hiding this comment.
Is this necessary? Has no effect on my machine, and messing with position when unnecessary often results in hard do maintain code...
| @@ -58,3 +58,35 @@ The buttons can also be used in combination with an icon. | |||
| Add something | |||
| </Button> | |||
| ``` | |||
|
|
|||
| The prob `showDropdownIcon` displays a drop down icon on the right side. | |||
There was a problem hiding this comment.
You have written prob instead of prop
|
|
||
| .dropdown-icon { | ||
| left: auto; | ||
| right: 10px; |
There was a problem hiding this comment.
Wasn't that easy to understand where the position attribute came from. I think I would not make this class dependent on the button-icon class, which means only setting one of both. To extract what they have in common you could still use a selector like .button-icon, .dropdown-icon.
| schemaKey: string, | ||
| label: string, | ||
| visibility: 'always' | 'yes' | 'no', | ||
| onChange: (schemaKey: string, visibility: 'yes' | 'no') => void, |
There was a problem hiding this comment.
Didn't we always have the value (in that case visibility) as the first parameter? Would keep it that way, in order for it to be more consistent.
| const page = observable.box(); | ||
| const datagridStore = new DatagridStore('snippets', {page}); | ||
| const datagridStore = new DatagridStore('snippets', 'datagrid_test', {page}); | ||
| datagridStore.schema = { |
There was a problem hiding this comment.
Wasn't the schema set with the promise above? Why does it have to be set twice in this test?
| type: 'string', | ||
| visibility: 'always', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
This test got very bloated, because this schema is added to every single test. Wouldn't it be enough to have different visibility only in one test, and in all others we use the minimum? Maybe even only the id, if that works?
Then it would be much less work to update if the schema changes.
| const webspace = userStore.getPersistentSetting(USER_SETTING_WEBSPACE); | ||
| static getDerivedRouteAttributes(route, attributes) { | ||
| const webspace = attributes.webspace ? attributes.webspace : | ||
| userStore.getPersistentSetting(USER_SETTING_WEBSPACE); |
There was a problem hiding this comment.
If we have to split it, there should be an own line for everything and move the operators in front of the line:
const webspace = attributes.webspace
? attributes.webspace
: userStore.getPersistentSetting(USER_SETTING_WEBSPACE);| @@ -109,7 +113,12 @@ class WebspaceOverview extends React.Component<ViewProps> { | |||
| router.bind('webspace', this.webspace); | |||
| apiOptions.webspace = this.webspace; | |||
|
|
|||
| this.datagridStore = new DatagridStore('pages', observableOptions, apiOptions); | |||
| this.datagridStore = new DatagridStore( | |||
| 'pages', | |||
There was a problem hiding this comment.
If you introduce a constant we should also use it here 😉
| const Datagrid = require('../Datagrid').default; | ||
| const userStore = require('../../../stores/UserStore'); | ||
| const DatagridStore = require('../../../containers/Datagrid').DatagridStore; | ||
| DatagridStore.getActiveSetting = jest.fn(); |
There was a problem hiding this comment.
Why don't you make this a class in the mock of this test as in one of the tests below?
What's in this PR?
Add column list options to datagrid.
Why?
User needs to select and sort the visible columns in the datagrid.