Skip to content
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

Adds clear all button in header using the FilterRemoveIcon Octicon #3778

Merged
merged 8 commits into from
Oct 8, 2023

Conversation

UnicodeRogue
Copy link
Contributor

@UnicodeRogue UnicodeRogue commented Sep 28, 2023

Closes #2399

Changelog

New

No new files added, did add a new story, called I With Remove Filter Icon

Changed

Imported visual button FilterRemoveIcon and created the functionality to clear items selected in the drop-down list

Removed

One file was removed, the temporary icon that was a placeholder until the official Octicon one

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Storybook)
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

⚠️ No Changeset found

Latest commit: 604cb46

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Sep 28, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.71 KB (0%)
dist/browser.umd.js 105.28 KB (0%)

@UnicodeRogue UnicodeRogue temporarily deployed to github-pages September 28, 2023 18:27 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 September 28, 2023 18:27 Inactive
@UnicodeRogue UnicodeRogue marked this pull request as ready for review September 28, 2023 18:31
@UnicodeRogue UnicodeRogue requested review from a team and joshblack September 28, 2023 18:31
@gr2m gr2m added the skip changeset This change does not need a changelog label Sep 28, 2023
package.json Outdated Show resolved Hide resolved
Co-authored-by: Gregor Martynus <[email protected]>
@UnicodeRogue UnicodeRogue temporarily deployed to github-pages September 28, 2023 19:42 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 September 28, 2023 19:43 Inactive
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure where the changes to examples/nextjs/package-lock.json came from. As far as I can tell we just ran npm install in the project root. Should we get the change removed from the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I'm okay to remove any changes 🙇🏻 You're right, what I know we didn't specifically work on I was thinking was coming through from the npm install/any updates around that

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is your fault, see #3776

@siddharthkp siddharthkp removed the request for review from joshblack October 2, 2023 09:39
@@ -1,5 +1,5 @@
import React from 'react'
import {SearchIcon, XCircleFillIcon, XIcon} from '@primer/octicons-react'
import {SearchIcon, XCircleFillIcon, XIcon, FilterRemoveIcon} from '@primer/octicons-react'
Copy link
Member

Choose a reason for hiding this comment

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

Very nice!

@@ -773,6 +773,64 @@ export const GOpenFromMenu = () => {
)
}

export const IWithRemoveFilterIcon = () => {
Copy link
Member

@siddharthkp siddharthkp Oct 2, 2023

Choose a reason for hiding this comment

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

Just checking, do we want to keep this story because clearSelection is also present in other stories

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so? If only because it will be easy to take out later if we want 🙇🏻

@@ -16,7 +16,7 @@
"@lit-labs/react": "^1.1.1",
"@oddbird/popover-polyfill": "0.2.2",
"@primer/behaviors": "^1.3.4",
"@primer/octicons-react": "^19.7.0",
"@primer/octicons-react": "19.8.0",
Copy link
Member

@siddharthkp siddharthkp Oct 2, 2023

Choose a reason for hiding this comment

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

Kinda odd that the lock file doesn't have a caret ^ anymore even though package.json does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not having a ^ was the suggested format when I ran updates, but I agree that the lines read a bit weird. I'm going to pull in any recent changes and see if that resolves things

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left some small nit picks :)

Approving in advance

@UnicodeRogue UnicodeRogue temporarily deployed to github-pages October 6, 2023 13:11 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 October 6, 2023 13:12 Inactive
@UnicodeRogue UnicodeRogue temporarily deployed to github-pages October 6, 2023 13:51 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 October 6, 2023 13:51 Inactive
@UnicodeRogue
Copy link
Contributor Author

All right, working through the last few bits here- I do see three currently failing CI tests, mostly about ActionList- I wonder if this is something we can take out?

Screenshot 2023-10-06 at 9 57 10 AM Screenshot 2023-10-06 at 9 57 22 AM

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2023

Uh oh! @UnicodeRogue, the image you shared is missing helpful alt text. Check #3778 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@gr2m gr2m temporarily deployed to github-pages October 8, 2023 18:36 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 October 8, 2023 18:36 Inactive
@gr2m gr2m temporarily deployed to github-pages October 8, 2023 18:44 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3778 October 8, 2023 18:44 Inactive
Copy link
Contributor

Choose a reason for hiding this comment

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

@gr2m gr2m added this pull request to the merge queue Oct 8, 2023
Merged via the queue into main with commit ae23932 Oct 8, 2023
31 checks passed
@gr2m gr2m deleted the UnicodeRogue-clear-all branch October 8, 2023 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants