[APP-526] feat: Use NBS components for Advanced Filter UI#3315
Conversation
emyl3
force-pushed
the
el/app-526/adv-filter-nbs-components
branch
2 times, most recently
from
2e9574c to
33c1d00
Compare
|
|
||
| const dateRangePickerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
There was a problem hiding this comment.
(q, nb): Given that, is the dateRangePickerRef still needed?
emyl3
force-pushed
the
el/app-526/adv-filter-nbs-components
branch
from
33c1d00 to
736da60
Compare
emyl3
requested review from
JordanGuinn and
brick-green
and removed request for
a team
| svg { | ||
| fill: colors.$error; | ||
| } | ||
|
|
||
| &:hover { | ||
| svg { | ||
| fill: colors.$error-dark; | ||
| } | ||
| } |
There was a problem hiding this comment.
(nit, maybe-b): can we use the "destructive" option instead of custom styling here?
another trash/delete icon in ManageSectionTile.tsx looks like
<Button
type="button"
className={styles.iconBtn}
outline
disabled={onAction}
onClick={() => {
setSelectedForDelete(section);
setOnAction(true);
}}
>
<Icon.Delete data-testId="deleteIcon" style={{ cursor: 'pointer' }} size={3} />
</Button>
I wonder if we should be coalescing on this format?
There was a problem hiding this comment.
I used the destructive prop on the button and removed that styling!
The button in ManageSectionTile.tsx looks slightly different than the designs so I am leaning towards keeping the current implementation as-is. What do you think?
Co-authored-by: Mary McGrath <m.c.mcgrath13@gmail.com>
| rules: [{ id: crypto.randomUUID(), field: '~', operator: '~', value: '' }], | ||
| }; | ||
|
|
||
| type NbsOperator = Operator & { nbsCd: string }; |
There was a problem hiding this comment.
JordanGuinn
left a comment
JordanGuinn
left a comment
There was a problem hiding this comment.
Handful of questions/thoughts, but this is looking good!!
| @@ -0,0 +1,17 @@ | |||
| import React from 'react'; | |||
| import { ActionProps } from 'react-querybuilder'; | |||
| import { Button } from '../../../../../design-system/button'; | |||
There was a problem hiding this comment.
(thought, super-nb): ENTIRELY unrelated to this PR, but it'd be great if we could get path aliasing working at some point, and remove the need to traverse up the codebase like this for some of our dependencies!
There was a problem hiding this comment.
Yes! Agree! Do you prefer I create a separate ticket to keep track of this or just a good thing to keep in mind when we are out of the crunch period? 👀
There was a problem hiding this comment.
A tech debt ticket couldn't hurt! Thanks Elisa 🙏
| import { ValueInput } from './ValueInput.tsx'; | ||
|
|
||
| const ValueEditor = (props: ValueEditorProps<ValueSetMetadata & FullField>) => { | ||
| const ValueEditor = (props: ValueEditorProps<ValueSetMetadata & FullField & FullOperator>) => { |
There was a problem hiding this comment.
(nit, nb): Thoughts on renaming our ValueEditor something that isn't identical to what's provided by react-querybuilder? Just to indicate that they are slightly different, and remove the potential for anyone using the react-querybuilder version by mistake
There was a problem hiding this comment.
Renamed to ValueEditorSwitch 😅 let me know if you have a different name in mind 👀
| if (isRuleType(rule)) { | ||
| const { id, field, operator, value, label } = rule; | ||
|
|
||
| if (!id) return; // no key for the map, shouldn't happen in practice |
There was a problem hiding this comment.
(nit, nb): I wonder if we want to log a warning or throw or something here, if this shouldn't happen in practice?
There was a problem hiding this comment.
added a console.warn! Thank you!!
| result[id] = { valid: true }; | ||
|
|
||
| // empty rules are fine | ||
| if (!field || field === '~') return; |
There was a problem hiding this comment.
(q, nb): Out of curiosity, why are empty rules fine? Do they get compacted at a later point prior to hitting the API?
There was a problem hiding this comment.
It gets ignored and we do not send an empty rule over.
| @@ -0,0 +1,65 @@ | |||
| export const validateDateRange = (value, field) => { | |||
| if (rangeValuesMissing(value)) return getRangeValErrorMsg(field, false); | |||
There was a problem hiding this comment.
(thought, nb): I feel like there's gotta be a way to assert the presence of value[0] and value[1] here with an assertion on rangeValuesMissing, but a thought for another time!
| const fromDate = new Date(value[0]!); // can't be undefined because of above checks | ||
| const toDate = new Date(value[1]!); // can't be undefined because of above checks | ||
| for (const { date, ind, str } of [ | ||
| { date: fromDate, ind: 0, str: 'From' }, |
There was a problem hiding this comment.
(nit, nb): Could we just reference the underlying value in the array, rather than including an index in the list we're iterating against?
Also, given this is just two entries, it could arguably be a bit simpler if it was just two if statements, without the for loop entirely?
There was a problem hiding this comment.
Took your first suggestion and passed the value directly instead of the index! Thank you!!
| return typeof val === 'number'; | ||
| }; | ||
|
|
||
| export const isDateFormat = (val) => { |
There was a problem hiding this comment.
(q, nb): I think date-fns supports something like this out of the box? Assuming it supports the formats we're matching against here
There was a problem hiding this comment.
Modified it to use date-fns to validate the date and caught some edge cases in the process -- thank you!!
|
@JordanGuinn @brick-green ready for re-review! |
Co-authored-by: Jordan Guinn <the.jordan.guinn@gmail.com>
3 participants
Description
Screenshot 📸
Tickets
Checklist before requesting a review