Skip to content

Conversation

@rickitan
Copy link
Contributor

@rickitan rickitan commented Mar 18, 2020

WHY are these changes introduced?

Fixes #2826

The filters component always shows a "More filters" button. If there's only one filter or we can fit all the filters in the container, the button feels unnecessary.

This image describes the problem:

image

Notice how there's only one filter, and we still show the "More filters" button.

WHAT is this pull request doing?

This PR introduces the following behaviour:

  • If there's no filters marked with shortcut: true we show the More filters button
  • If all the filters are marked with shortcut: true and we can fit them in the container, we don't show the More filters button
  • If all the filters are marked with shortcut: true and we CAN'T fit them in the container, we show the More filters button
  • If some of the filters are marked with shortcut: true but no all, we show the More filters button

This shows the new look:
image

The changes are backwards compatible.

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@ghost
Copy link

ghost commented Mar 18, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2020

🟢 This pull request modifies 7 files and might impact 2 other files.

Details:
All files potentially affected (total: 2)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Filters/Filters.tsx (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Filters/components/ConnectedFilterControl/ConnectedFilterControl.scss (total: 2)

Files potentially affected (total: 2)

🧩 src/components/Filters/components/ConnectedFilterControl/ConnectedFilterControl.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/Filters/components/ConnectedFilterControl/tests/ConnectedFilterControl.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Filters/tests/Filters.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/MediaCard/MediaCard.tsx (total: 0)

Files potentially affected (total: 0)

@rickitan rickitan force-pushed the hide_more_filters_button_prop branch from bbe9958 to 46005d4 Compare March 19, 2020 14:06
@rickitan rickitan requested review from chloerice and tmlayton March 19, 2020 14:36
@tmlayton
Copy link
Contributor

I like the approach, only show more filters if there are actually more filters! 😄

There is some polishing work to do. A couple things I noticed from a quick 🎩 are:

  • Last button when hiding more filters does not have rounded corners
    Screen Shot 2020-03-19 at 12 48 54 PM
  • Some funky behavior here where more filters shows up even though the button is larger:

    measuring

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

See above comment

@rickitan
Copy link
Contributor Author

Hi @tmlayton I just addressed you comments. Let me know what you think!

@tmlayton
Copy link
Contributor

Looking better, still some styling needed to fix up connected buttons. Noticed the rounded corners on the right side of the "Tagged with" button:

Screen Shot 2020-03-27 at 12 51 36 PM

Screen Shot 2020-03-27 at 12 51 46 PM

@rickitan
Copy link
Contributor Author

Hi tim! @tmlayton

I just address the border-radius problem. Let me know what you think!

@rickitan rickitan requested a review from tmlayton March 31, 2020 14:29
@tmlayton
Copy link
Contributor

Looks good!

@rickitan rickitan force-pushed the hide_more_filters_button_prop branch from a1fdf6c to 26a002a Compare April 8, 2020 16:00
@rickitan rickitan changed the title [Filters] adds a hideMoreFiltersButton prop to the Filters component [Filters] only show the "More filters" button if necessary Apr 8, 2020
@tmlayton
Copy link
Contributor

tmlayton commented Apr 8, 2020

For test coverage run dev test:coverage or in this case dev test:coverage ConnectedFilterControl.test.tsx which will generate and open the report in the browser.

Looking at the codecov diff the lines without coverage are flagged in yellow for partial or red for none:

Screen Shot 2020-04-08 at 11 10 00 AM

Screen Shot 2020-04-08 at 11 10 06 AM

Adding tests for this is fairly straightforward. The two cases are when shouldRenderMoreFiltersButton is false and when only some buttons fit and the loop broken.

Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Couple small changes, otherwise this is ready to go. I went ahead and pushed a couple more tests for code coverage.

@rickitan rickitan force-pushed the hide_more_filters_button_prop branch from e64eef5 to 06945aa Compare April 8, 2020 20:56
@tmlayton tmlayton merged commit 2c486bc into master Apr 9, 2020
@tmlayton tmlayton deleted the hide_more_filters_button_prop branch April 9, 2020 21:36
@ghost
Copy link

ghost commented Apr 9, 2020

🎉 Thanks for your contribution to Polaris React!

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.

Feature request : making visibility of "More Filters" button in Filters component configurable

3 participants