[WIP][dashboard scoped filter] part 1: scope selector modal#8557
Conversation
etr2460
left a comment
etr2460
left a comment
There was a problem hiding this comment.
reviewed again. these are mostly spelling, grammar, and naming nits, so i'm going to approve to unblock. However, I would appreciate you addressing these comments before merging. Thanks!
| const propTypes = { | ||
| activeKey: PropTypes.oneOfType([null, PropTypes.string]), | ||
| nodes: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired, | ||
| checked: PropTypes.arrayOf( |
There was a problem hiding this comment.
I remember your comment from the previous PR, what this really is is checkedIds or checkedList. checked by itself seems very much like a boolean argument. I know that's what the library uses, but can we update to one of the more explicit prop names to make this clearer?
I still don't understand why this needs to allow both numbers and strings too. Can we enforce one or the other coming in so that we don't have this multityped array?
| checked: PropTypes.arrayOf( | ||
| PropTypes.oneOfType([PropTypes.number, PropTypes.string]), | ||
| ).isRequired, | ||
| expanded: PropTypes.arrayOf( |
| nodes = [], | ||
| checked = [], | ||
| expanded = [], |
There was a problem hiding this comment.
all of these are marked as required in proptypes, so i think we don't need these default values
| // Node's label matches the search string | ||
| node.label.toLocaleLowerCase().indexOf(searchText.toLocaleLowerCase()) > | ||
| -1 || | ||
| // Or a children has a matching node |
There was a problem hiding this comment.
sp nit: or a child has a matching node
| {currentFilterLabels.length === 0 && t('No filter is selected.')} | ||
| {currentFilterLabels.length === 1 && t('Editing 1 filter:')} | ||
| {currentFilterLabels.length > 1 && | ||
| t('Batch editing %d filters:', currentFilterLabels.length)} |
| &.root { | ||
| font-weight: 700; | ||
| } | ||
|
|
||
| &.tab { | ||
| font-weight: 700; | ||
| } |
There was a problem hiding this comment.
i think this can be:
&.root, &.tab {
font-weight: 700;
}| top: 16px; | ||
| border-radius: 4px; | ||
| border: 1px solid #ccc; | ||
| padding: 4px 8px 4px 8px; |
| } | ||
|
|
||
| // currently filterbox is a chart, | ||
| // when define filter scopes, they have to be out pulled out in a few places. |
There was a problem hiding this comment.
sp nit: when defining filter scopes...
|
|
||
| // currently filterbox is a chart, | ||
| // when define filter scopes, they have to be out pulled out in a few places. | ||
| // after we make filterbox a dashboard build-in component, |
| components = {}, | ||
| filterFields = [], |
There was a problem hiding this comment.
i don't think we need these defaults since the function below already defaults them
* [WIP][dashboard scoped filter] part 1: scope selector modal (#8557) * filter scope selector modal * add single-field-edit in multi-edit mode switch * fix code review comments (round 1) * refactory after design review * fix a few props initial value * [WIP][dashboard scoped filter] part 2: add algorithm to convert checked ids to scope object (#8564) * convert ids to scope object * use lodash helpers to make code readable * [WIP][dashboard scoped filter] part 3: merge filter scope settings into dashboard redux state (#8522) * merge filter scope settings into dashboard redux state * fix/add unit tests * minor bug fixes * fix save filter Scopes behavior * resolve review comments * fix save filter scope settings * minor comments * [dashboard scoped filter] Improve scrollbar inside modal (#8553) * improve scroll inside modal * make left pane and right pane scroll separately * fix review comments * force show filter_box as unchecked (#8587)
3 participants
This is PR #8404 rebased onto latest master.