(CISC-2409) Quick filter with no options #553
Conversation
Jo-Lillie
force-pushed
the
cisc-2409-quick-filter-0with-no-items
branch
2 times, most recently
from
5691f88 to
b486bf7
Compare
| return ( | ||
| <div className="dg-quick-filter-container"> | ||
| <div className="dg-quick-filter-filters"> | ||
| {filters.map((filter, idx) => { | ||
| return ( | ||
| return !filter.options ? ( |
There was a problem hiding this comment.
Rather than having two Button selects that we can return, we can just do some condition logic in the props. Something like options={filter.options.length !== 0 ? emptyFilterOption : filter.options}.
I also think we should check on the array length of options rather than if options is provided. A querry will normally return an empty array rather than returning nothing so checking the length will prevent the empty state from showing not showing on an empty array.
There was a problem hiding this comment.
We definitely need to check the length of the array (if it exists). Good catch.
Whether to have two Button selects or use conditions ... I believe the existing approach is more readable, while your suggestion is more concise and not so prone to errors.
| return ( | ||
| return !filter.options ? ( | ||
| <ButtonSelect | ||
| className="dg-quick-filter-empty" |
There was a problem hiding this comment.
we can do condition logic for classnames to using an npm package called classnames https://www.npmjs.com/package/classnames
| @@ -7,6 +9,19 @@ | |||
| padding-right: 8px; | |||
| padding-bottom: 16px; | |||
| } | |||
| .dg-quick-filter-empty{ | |||
| padding-right: 8px; | |||
There was a problem hiding this comment.
using classname you can give a filter both the .dg-quick-filter-filters and .dg-quick-filter-empty classnames. That means you will not need to repeat the padding-right and padding bottom values.
There was a problem hiding this comment.
Agree. I must admit I noticed that as well but didn't consider it important. But when you write it down, I can clearly see it makes sense.
| @@ -49,6 +49,10 @@ const filters = [ | |||
| }, | |||
| ], | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
I get this component doesn't currently have any written docs and im not asking you to completely write a whole page of content. But I think we need to show this example separate from the rest and explain this is only a disabled state. And that a null value is an acceptable one value and should be treated differently. Would even be worth showing the null value example from comply.
| /** Used for the options array when there is no items to filter by */ | ||
| const emptyFilterOption = [ | ||
| { | ||
| label: 'No items to filter by', |
There was a problem hiding this comment.
Do you think it makes sense to name this as disabledFilterOption as apposed to empty? Also the label string needs to be configurable so pass in a prop
There was a problem hiding this comment.
I personally prefer the empty word as opposed to disabled because the filter is in fact empty, not disabled...
Sure, the label string can be configurable, good idea...
|
@Jo-Lillie hi! Just saw this PR and wanted to request a potential change: Could we make the |
Jo-Lillie
force-pushed
the
cisc-2409-quick-filter-0with-no-items
branch
from
b486bf7 to
1726482
Compare
|
@Lukeaber @tomasholub @chrisleicester That should be all the comments addressed, except for the docs which I will be working on. Please when you have time re-review. Thanks! :) |
| return ( | ||
| <div className="dg-quick-filter-container"> | ||
| <div className="dg-quick-filter-filters"> | ||
| {filters.map((filter, idx) => { | ||
| return ( | ||
| <ButtonSelect | ||
| className="dg-quick-filter" | ||
| className={classnames('dg-quick-filter-filter', { | ||
| 'dg-quick-filter': true, |
There was a problem hiding this comment.
If this is always going to be true you can just have it declared in the function instead of within an object and with logic. classnames('dg-quick-filter-filter', 'dg-quick-filter', {'dg-quick-filter-empty': filter.options.length === 0, })}
There was a problem hiding this comment.
Thanks @Lukeaber - This has now been updated along with the added README file which is hopefully up to standard 😁
Jo-Lillie
force-pushed
the
cisc-2409-quick-filter-0with-no-items
branch
from
1726482 to
eb10e67
Compare
Jo-Lillie
force-pushed
the
cisc-2409-quick-filter-0with-no-items
branch
from
eb10e67 to
e306afc
Compare
Description of proposed changes
Before if there were no items to filter by in the quick filter it was still clickable but nothing happened. These changes are to show that there are no items to filter by when the options array is empty.
Also adds README file for the quick filter.
JIRA TICKET
Figma design
Screenshot of proposed changes