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

refactor: adopt primer design system ui components #1589

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

Conversation

setchy
Copy link
Member

@setchy setchy commented Oct 12, 2024

Closes: #1541

Adopts the excellent GitHub Primer Design System Component Library - https://primer.style/components to replace our custom components and provide a level-up on user experience and visual consistency.

This PR, though sizeable, focuses on replacing the following components

  • Icons
  • Buttons
  • Layout via Stack

I have also replaced most tests to use data-testid and getByTestId so that they're more resilient to UI changes

Items found during testing that should be resolved

  • Horizontal scroll bar showing on notification animation (mark as read, etc)
  • Different uses of colors (tailwind vs design token), eg: green color
  • position of repository title tooltip gets cut off

Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@github-actions github-actions bot added dependency Dependency updates refactor Refactoring of existing feature labels Oct 12, 2024
setchy added 11 commits October 13, 2024 11:01
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@bmulholland
Copy link
Collaborator

I have also replaced most tests to use data-testid and getByTestId so that they're more resilient to UI changes

Lovely, nice touch.

jest.setup.js.js Outdated Show resolved Hide resolved
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy setchy marked this pull request as ready for review October 14, 2024 22:05
@setchy setchy requested a review from afonsojramos as a code owner October 14, 2024 22:05
@setchy
Copy link
Member Author

setchy commented Nov 7, 2024

Appreciate the kind words @afonsojramos and the feedback on some visual inconsistencies. I'll address them shortly 👨‍💻

@setchy setchy marked this pull request as draft November 27, 2024 12:29
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Dec 3, 2024

However, I've found a couple of visual issues:

Apologies this took a while to get back to. Items 1, 2 and 3 have been addressed

I think we're getting quite close to this being merged. 6.x.x milestone perhaps?

@setchy setchy requested a review from afonsojramos December 3, 2024 04:27
@setchy setchy marked this pull request as ready for review December 3, 2024 14:24
@afonsojramos
Copy link
Member

@setchy do you want some help in the replacement of the sxs?

@setchy
Copy link
Member Author

setchy commented Dec 6, 2024

@setchy do you want some help in the replacement of the sxs?

Absolutely! Always welcome a friendly hand.

I haven't attempted to remove the style components used with primer components... I'm not sure how feasible that will be, but if possible I'm keen

@afonsojramos
Copy link
Member

afonsojramos commented Dec 6, 2024

Seems like it is not possible 😅 We need to override the component's defaults... Oh well 🤷 It's annoying, but isn't much of a problem.

@setchy
Copy link
Member Author

setchy commented Dec 6, 2024

Thanks for taking a look 🙏

@setchy setchy added this to the Release 6.0.0 milestone Dec 26, 2024
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Jan 17, 2025

@bmulholland @afonsojramos - I've resolved all merge conflicts and done some final refactoring to our tailwind colors.

I think I'm ready for final review from you both if you have some time 🙏

There is one niggling issue remaining whereby the user-selected theme is not correctly loading on startup...

Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Jan 17, 2025

There is one niggling issue remaining whereby the user-selected theme is not correctly loading on startup...

This is now resolved. Left a few TODO comments for future us to clean up some of the legacy theming code

Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Dependency updates refactor Refactoring of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

explore adopting GitHub's primer.design component library
3 participants