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.

Sign up for GitHub

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

Jump to bottom

Icons: add thumbs up down icons #566

Merged
merged 5 commits into from
Jul 26, 2023
Merged

Icons: add thumbs up down icons #566

merged 5 commits into from
Jul 26, 2023

Conversation

rehanabbasi
Copy link
Contributor

This change: (check at least one)

  • Adds a new feature
  • Fixes a bug
  • Improves maintainability
  • Improves documentation
  • Is a release activity

Is this a breaking change? (check one)

  • Yes
  • No

Is the: (complete all)

  • Title of this pull request clear, concise, and indicative of the issue number it addresses, if any?
  • Test suite(s) passing?
  • Code coverage maximal?
  • Changeset added?
  • Component status page up to date?

What does this change address?
Adds thumbs up/down icons

@rehanabbasi rehanabbasi requested a review from a team as a code owner July 25, 2023 18:35
@rehanabbasi rehanabbasi requested review from SMQuazi, mtorres3 and satya-achanta-venkata and removed request for a team July 25, 2023 18:35
@changeset-bot
Copy link

changeset-bot bot commented Jul 25, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: dc59601

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ithaka/pharos Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 25, 2023 •
edited
Loading

size-limit report 📦

Path Size
packages/pharos/lib/index.js 47.05 KB (0%)

@daneah
Copy link
Member

daneah commented Jul 25, 2023

@rehanabbasi you'll need to remove the enclosing <svg> as those get added dynamically in the code; see the linting job failure for more detail. You should be able to run yarn lint locally to see this as well.

@chrisjbrown chrisjbrown self-requested a review July 25, 2023 19:09
@chrisjbrown
Copy link
Contributor

@rehanabbasi do you need to supply filled and unfilled icons? or can the consumer decide that?

@rehanabbasi
Copy link
Contributor Author

@rehanabbasi do you need to supply filled and unfilled icons? or can the consumer decide that?

@chrisjbrown I had specified unique icon names for each of those variants!

@chrisjbrown
Copy link
Contributor

@rehanabbasi do you need to supply filled and unfilled icons? or can the consumer decide that?

@chrisjbrown I had specified unique icon names for each of those variants!

I mean to say
are those variants necessary?
would it be better to allow the consumer to style those icons

@rehanabbasi
Copy link
Contributor Author

@rehanabbasi do you need to supply filled and unfilled icons? or can the consumer decide that?

@chrisjbrown I had specified unique icon names for each of those variants!

I mean to say are those variants necessary? would it be better to allow the consumer to style those icons

Understood - so I think we already have filled and unfilled icon example in the font set (checkmark-filled-circle and checkmark-inverse ) where we provide separate icons for each filled and unfilled variant - so was following that!

@daneah
Copy link
Member

daneah commented Jul 25, 2023

@rehanabbasi @chrisjbrown yeah the key difference here being that these icons actually change nature when filled or not, instead of just changing color. I think it makes sense as well.

@chrisjbrown
Copy link
Contributor

@rehanabbasi @chrisjbrown yeah the key difference here being that these icons actually change nature when filled or not, instead of just changing color. I think it makes sense as well.

yeah when i actually looked at the icon didn't seem like any way to get that desired fidelity via the consumer. looks good now

@daneah daneah merged commit ea25a41 into develop Jul 26, 2023
8 checks passed
@daneah daneah deleted the feature/thumbs-up-down-icons branch July 26, 2023 12:44
@github-actions github-actions bot mentioned this pull request Jul 26, 2023
Merged
@rehanabbasi rehanabbasi restored the feature/thumbs-up-down-icons branch July 27, 2023 21:40
sirrah-tam pushed a commit to sirrah-tam/pharos that referenced this pull request Dec 1, 2023
@rehanabbasi @sirrah-tam
Icons: add thumbs up down icons (ithaka#566)
b15f643 
* feat(icons): add thumbs up down icons

* docs(icons): update change log for adding thumbs up down icons

* feat(icons): remove enclosing svg tags in the asset files

* feat(icons): remove enclosing fill attribute on the svg content

* feat(icons): remove rect tag from svg files
@daneah daneah mentioned this pull request Mar 8, 2024
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daneah daneah daneah approved these changes

@chrisjbrown chrisjbrown chrisjbrown approved these changes

@SMQuazi SMQuazi Awaiting requested review from SMQuazi SMQuazi is a code owner automatically assigned from ithaka/pharos-maintainers

@mtorres3 mtorres3 Awaiting requested review from mtorres3 mtorres3 is a code owner automatically assigned from ithaka/pharos-maintainers

@satya-achanta-venkata satya-achanta-venkata Awaiting requested review from satya-achanta-venkata satya-achanta-venkata is a code owner automatically assigned from ithaka/pharos-maintainers

Assignees
No one assigned
Labels
component/icon status/ready to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rehanabbasi @daneah @chrisjbrown