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 46 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

@github-actions github-actions bot added dependency Dependency updates refactor Refactoring of existing feature labels Oct 12, 2024
@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 <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
@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
Signed-off-by: Adam Setch <[email protected]>
@bmulholland
Copy link
Collaborator

I think open in background is broken on this branch

@setchy
Copy link
Member Author

setchy commented Oct 28, 2024

I think open in background is broken on this branch

Just tested and it's working for me...

@bmulholland
Copy link
Collaborator

Will do some more testing

@setchy
Copy link
Member Author

setchy commented Oct 31, 2024

How has the testing been going @bmulholland ? For me it's been really stable.

Once this is merged I plan to (finally) move forward with #985 - I have configured this on the atlassify codebase so should be an easy port across

@bmulholland
Copy link
Collaborator

Yeah it's been working well for me :)

@setchy
Copy link
Member Author

setchy commented Oct 31, 2024

@afonsojramos - I know you're a busy man atm, but if you do happen to have time to shake this down would love your feedback, too

@afonsojramos
Copy link
Member

Giving it a try! 👀

@afonsojramos
Copy link
Member

Wow, this has been a huge amount of work! Everything looks great!

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

  1. Input component hover border is red by default?
image
  1. Spacing in some icons under accounts seems too tight. I imagine my name is too long 😅 If it was my full name it would be even worse 🧨
image
  1. This weird hover in this text and icons under Accounts.
image

And got a few questions regarding implementation on some things. I know that to add primer/react we needed to add styled-components, but I don't think we should mix and match both styled-components and tailwind. So I'd probably change those sx's.

I'm also (sometimes) seeing some errors on the console, but I think that is related to this: https://styled-components.com/docs/faqs#shouldforwardprop-is-no-longer-provided-by-default
image

@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
@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 🙏

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