Skip to content

Conversation

JCoder121
Copy link
Contributor

@JCoder121 JCoder121 commented Mar 24, 2023

Complex Story Deploy Preview

What does this PR do?

Add functionality to Data's filter() for when a property is a list, rather than a string, bool, or nested object. Allow the user to select filter values that filter upon list objects.

Where should the reviewer start?

src/js/components/Data/filter.js
src/js/components/Data/stories/Complex.js

What testing has been done on this PR?

Manual and visual testing to ensure filtering upon list elements works as intended.

How should this be manually tested?

Check that existing filtering works as expected in addition to the new list filtering capabilities.

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Closes #6701

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes - update after initial code review to include example case.

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality looks good! Couple small cleanup comments.

@ericsoderberghp
Copy link
Contributor

Screenshot 2023-03-24 at 12 21 52 PM

Something looks off with the vertical alignment here. I don't think this is related to the changes in this pull request but wondering if you can take a quick look.

@ericsoderberghp
Copy link
Contributor

Can you enhance the story so that searching matches on the color names, in addition to the filtering?

@ericsoderberghp
Copy link
Contributor

I think we should consider using verticalAlign for the columns to align them to the top instead of centering the cell contents.

@JCoder121
Copy link
Contributor Author

JCoder121 commented Mar 24, 2023

Screenshot 2023-03-24 at 12 21 52 PM

Something looks off with the vertical alignment here. I don't think this is related to the changes in this pull request but wondering if you can take a quick look.

image

This screenshot is from the Views story here: https://storybook.grommet.io/?path=/story/data-data-views--views. It's looking like whenever the `Clear Filters` button is inserted in the toolbar, it isn't centered inside its parent Box (maybe aligned to start?). Additionally, when tabbing through the controls, the Filter button's focus indicator box seems to be very slightly larger than the DataSearch component (and in this case the Select component as well).

Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Enhancement with Array
3 participants