-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Implement new filters panel #18449
Conversation
@volodymyr-melnykc Could you take a look into this locally? |
72392be
to
7fdf0b7
Compare
tasklist/client/src/Tasks/CollapsiblePanel/SearchParamNavLink/styles.module.scss
Outdated
Show resolved
Hide resolved
currentParams.entries(), | ||
); | ||
const values: Record<string, string> = | ||
(sortBy === 'completion' && !['custom', 'completed'].includes(sortBy)) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be simplified to sortBy === 'completion'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there's a bug. It should be (sortBy === 'completion' && !['custom', 'completed'].includes(filter))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll actually use you suggestion, it will make it more complicated than it needs to be if do what I originally wanted to do
if (filter === 'custom') { | ||
const customFilters = getStateLocally('customFilters')?.custom; | ||
const customFiltersParams = | ||
filter === 'custom' && customFilters !== undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter === 'custom'
always evaluates to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it was a copy and paste mistake
Object.keys(customFiltersParams).length > 0 | ||
? new URLSearchParams({ | ||
...convertedParams, | ||
...values, | ||
...customFiltersParams, | ||
}) | ||
: new URLSearchParams({ | ||
...convertedParams, | ||
...values, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be simplified to
new URLSearchParams({
...convertedParams,
...values,
...customFiltersParams
})
The key length check seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there were some type issues before on the old implementation but on this one I do some manipulation before so it's not really necessary anymore
sortBy === undefined | ||
? {filter} | ||
: {filter, sortBy}; | ||
if (filter === 'custom') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would extract this as a function which returns an object with the values to update. That way you don't have to repeat the params to delete.
const {filter, userId, currentParams} = options;
const {sortBy, ...convertedParams} = Object.fromEntries( currentParams.entries() );
const customFilterParams = filter === 'custom' ? getCustomFilterParams( userId ) : {};
const updatedParams = new URLSearchParams({
filter,
...convertedParams,
...values,
...customFilterParams,
});
if (sortBy !== undefined && sortBy !== 'completion') {
updatedParams.set('sortBy', sortBy);
}
if (filter === 'completed') {
updatedParams.set('sortBy', 'completion');
}
difference(
CUSTOM_FILTERS_PARAMS,
Object.keys(customFiltersParams),
).forEach((param) => {
updatedParams.delete(param);
});
return updatedParams.toString();
activeParam={{ | ||
key: 'filter', | ||
value: 'all-open', | ||
}} | ||
isActiveOnEmpty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on having a isActive
prop? This is the only instance that uses isActiveOnEmpty
.
isActive={filter === 'all-open' || filter === null}
Just seems like you're passing a lot of data to do this comparison in the Link component rather than here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, this is better
7fdf0b7
to
c828ffb
Compare
@douglasbouttell-camunda Should be ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some things to think about as you progress.
return updatedParams.toString(); | ||
} | ||
|
||
const CollapsiblePanel: React.FC = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're trying to build a collapsible menu here. I would start with markup that looks like:
<nav>
<ul>
<li>
<a aria-owns="filters-menu" aria-expanded="false">Filters <Icon/></a>
<ul role="group" id="filters-menu">
<li>...</li>
</ul>
<li>
<li>
<a aria-owns="custom-filters-menu" aria-expanded="false">Custom Filters <Icon/></a>
<ul role="group" id="custom-filters-menu">
<li>...</li>
</ul>
<li>
</ul>
</nav>
The rest is styles.
I'm fine to keep it like this for now but we should make a proper menu component in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@douglasbouttell-camunda I can do that, but this menu will only have one group for the foreseeable future
&.twoColumns { | ||
grid-template-columns: | ||
layout.to-rem(312px) | ||
1fr !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the !important
necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to make sure it wouldn't break the layout with the flag disabled, but this feature flag won't live for long so I'll just remove for now
c828ffb
to
9d6914f
Compare
9d6914f
to
81dc32e
Compare
Description
Creates feature flagged side panel with new filters UI
Related issues
related #17714