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

Aligning icons to Hypothicons library #1849

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jaredpdesigns
Copy link
Contributor

@jaredpdesigns jaredpdesigns commented Jan 15, 2025

PR aligns icons from Figma library to what's in the Pattern library. All base .svg files have been consistently formatted and have the same properties except those noted below.


Deprecated icons:

  • AnnotateAlt
  • CheckboxOutline
  • GlobeAltLock
  • LockAltFilled
  • LockAlt
  • ReplyAlt
  • SortAlt
  • TagAltFilled
  • TagAlt

I kept the generated .tsx files in place adding an @deprecated note, and created a new directory within images/icons and components/icons called _deprecated. Purpose here is to no longer export deprecated icons from the components/icons/index.ts file, which is auto-generated if there is an icon in the primary directory.


Breaking change:
There was were two export name changes that would require a separate PR for the client repo. I created a branch, but didn't have permission to publish.

Export name changes:

  • GlobeAltIcon -> GlobeIcon
  • GlobeAltLockIcon -> GlobeLockIcon

Files affected:

  • src/sidebar/components/GroupList/GroupListItem.tsx
  • src/sidebar/components/GroupList/test/GroupListItem-test.js

Outstanding questions/comments:

  • I didn't adjust MenuCollapse or MenuExpand as they appear to be slightly larger versions of CaretDown/CaretUp—I don't understand why these exist or in what context a larger variant is needed.
  • I didn't adjust SpinnerCircle or SpinnerSpokes, these feel like they should be a separate component, not part of the icon library.

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0255544) to head (5b387b9).

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1849   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           70        70           
  Lines         1303      1303           
  Branches       482       482           
=========================================
  Hits          1303      1303           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jaredpdesigns jaredpdesigns marked this pull request as ready for review January 15, 2025 18:17
@acelaya
Copy link
Contributor

acelaya commented Jan 16, 2025

I would prefer the "globe-lock" icon to keep a more generic name. Using it for restricted groups is one use-case, but we could find others in future, and it would feel weird to use an icon named "group-restricted" anywhere else.

@acelaya
Copy link
Contributor

acelaya commented Jan 16, 2025

It would be great to test the icons that have been adjusted everywhere where we are using them, as there have been icon changes in the past that looked reasonable in isolation, but then they had to be adjusted when used in actual apps.

@jaredpdesigns
Copy link
Contributor Author

Thanks for your feedback @robertknight I will adjust the name back to GlobeLock. Do you have a suggestion for thoroughly testing these in different environments other than the Pattern library? For example, can we cut a beta release and test in multiple repos that consume the library?

@acelaya
Copy link
Contributor

acelaya commented Jan 17, 2025

Do you have a suggestion for thoroughly testing these in different environments other than the Pattern library?

We usually use yalc to locally test changes in the frontend-shared library before releasing them. It is documented here https://github.com/hypothesis/frontend-shared/blob/02555449419a7520321bda7d0c5b9c0fe48b030c/docs/developing.md#testing-locally-with-other-projects

@robertknight
Copy link
Member

We usually use yalc to locally test changes in the frontend-shared library before releasing them

This would require Jared to get a complete Hypothesis development environment working, which is not trivial, especially for the LMS. We'll have to help with this stage.

@acelaya
Copy link
Contributor

acelaya commented Jan 17, 2025

We usually use yalc to locally test changes in the frontend-shared library before releasing them

This would require Jared to get a complete Hypothesis development environment working, which is not trivial, especially for the LMS. We'll have to help with this stage.

Yeah, that's true. Probably releasing a beta would have the same problem, so the most pragmatic approach is that one of us tests the changes.

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.

3 participants