ng-clarity icon indicating copy to clipboard operation
ng-clarity copied to clipboard

feat(datagrid): add possibility to clear filters

Open derkoe opened this issue 2 years ago • 21 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)
    Added it to the demo page.
    I could not find the sources for https://clarity.design/documentation/datagrid/structure
  • [ ] If applicable, have a visual design approval

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [x] Feature
  • [ ] Code style update (formatting, local variables)
  • [ ] Refactoring (no functional changes, no api changes)
  • [ ] Build related changes
  • [ ] CI related changes
  • [ ] Documentation content changes
  • [ ] Other... Please describe:

What is the current behavior?

Now, you cannot easily clear a filter. For string filters you have to delete all chars in the field, for number filters you have to click both fields and clear them

Issue Number: #225

What is the new behavior?

There is a new clear button added to the filter in the Datagrid which clears the current filter

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

derkoe avatar Jul 19 '22 07:07 derkoe

👋 @derkoe,

  • 🙏 The Clarity team thanks you for opening a pull request
  • 🎉 The build for this PR has succeeded
  • 🔍 The PR is now ready for review
  • 🍿 In the meantime, checkout out a preview of this PR
  • 🖐 You can always follow up here. If you're a VMware employee, you can also reach us on our internal #clarity-support Slack channel

Thank you,

🤖 Clarity Release Bot

github-actions[bot] avatar Jul 19 '22 07:07 github-actions[bot]

The visual regression test is failing because of a bug in the workflow for forks. I am going to look into fixing it.

kevinbuhmann avatar Jul 19 '22 15:07 kevinbuhmann

We need to get @colinreedmiller involved since this includes a visual change (and a slightly breaking one at that)

ashleyryan avatar Jul 19 '22 16:07 ashleyryan

'Does this PR introduce a breaking change?' checkbox indicates no breaking change - does need to be updated? Please add me as a reviewer.

colinreedmiller avatar Jul 19 '22 16:07 colinreedmiller

@colinreedmiller this PR came out of this discussion - I think it's a visual breaking change based on the screenshot?

https://github.com/vmware-clarity/ng-clarity/discussions/225

ashleyryan avatar Jul 19 '22 19:07 ashleyryan

@colinreedmiller this PR came out of this discussion - I think it's a visual breaking change based on the screenshot?

There is no breaking change it the API - it only adds the "Clear" button to the built-in filters (hence the visual change). Custom filters do not get the "Clear" button since they have to implement the clear(): void method.

derkoe avatar Jul 19 '22 19:07 derkoe

Thanks for the screenshot @ashleyryan - Yes, this is visually breaking and a design change more generally. UX needs to look at this before moving ahead.

colinreedmiller avatar Jul 19 '22 19:07 colinreedmiller

Is it possible to share the visual design via a screenshot? I need to see a working demo before I can provide feedback.

Partick avatar Jul 29 '22 17:07 Partick

I think we have filters in the demo app, but this build is failing. @derkoe, please fix the build errors, so that @Partick can see a live demo via the preview link.

kevinbuhmann avatar Jul 29 '22 17:07 kevinbuhmann

Fixed the build - @Partick you can see the working demo here https://246--storybook-clarity-design.netlify.app/?path=/story/datagrid-column--default

derkoe avatar Jul 29 '22 20:07 derkoe

@Partick any updates on this?

derkoe avatar Aug 17 '22 06:08 derkoe

it has been a month since the contribution attempt ... can we help speed things up a little?

d-m-s avatar Aug 19 '22 14:08 d-m-s

Is it possible to share the visual design via a screenshot? I need to see a working demo before I can provide feedback.

also discussed upfront: https://github.com/vmware-clarity/ng-clarity/discussions/225

working demo as already posted here https://246--storybook-clarity-design.netlify.app/?path=/story/datagrid-column--default

d-m-s avatar Aug 19 '22 14:08 d-m-s

Heads up on this - there has been a lot of PTO with summer vacations the last month or so. Design is currently looking at this, there's some concerns about the popup in a popup behavior

ashleyryan avatar Aug 26 '22 14:08 ashleyryan

Design is currently looking at this, there's some concerns about the popup in a popup behavior

Which popup? This change only adds the trash can to clear the filter.

derkoe avatar Aug 26 '22 15:08 derkoe

Which popup? This change only adds the trash can to clear the filter.

Oops, I commented on the wrong PR. Design is looking at this one too

ashleyryan avatar Aug 26 '22 15:08 ashleyryan

BTW this the current UX of clearing filters. For numbers you have to first focus the lower bounds, clear the field, then focus the upper bound and clear the field; then close the filter.

Clarity-Datagrid-clear-filter

derkoe avatar Aug 29 '22 05:08 derkoe

Any news on this?

If this will not be merged, can we at least export CustomFilter from Clarity and make smartToggleService protected? Then we can create our own clearable datagrid filter component.

derkoe avatar Oct 05 '22 07:10 derkoe

@colinreedmiller As you're currently the assigned person I'd like to ask how we can help to speedup the progress a little bit? Thanks :)

poifir avatar Nov 28 '22 15:11 poifir

@colinreedmiller As you're currently the assigned person I'd like to ask how we can help to speedup the progress a little bit? Thanks :)

Hello @Partick - I just noticed you're in the lead now :D - please tell me what is missing to get this PR merged. Thanks :)

poifir avatar Dec 06 '22 14:12 poifir

any updates to this issue? @Partick

d-m-s avatar Jan 09 '23 15:01 d-m-s