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

chore!: update styled-components to 6 & update DS to 2.0.0-beta.3 #20260

Merged
merged 18 commits into from May 7, 2024

Conversation

joshuaellis
Copy link
Member

@joshuaellis joshuaellis commented May 3, 2024

What does it do?

  • updates styled-components to v6
  • integrates changes from design-system 2.0.0-beta.3

Related issue(s)/PR(s)

  • resolves DX-1398
  • resolves DX-1253

@joshuaellis joshuaellis added flag: 💥 Breaking change This PR contains breaking changes and should not be merged source: dependencies Source is dependency problem flag: don't merge This PR should not be merged at the moment pr: chore This PR contains chore tasks (cleanups, configs, tooling...) labels May 3, 2024
@joshuaellis joshuaellis self-assigned this May 3, 2024
@joshuaellis joshuaellis force-pushed the chore/update-ds-2.0.0-beta.3 branch from 09f63d1 to 94314e1 Compare May 3, 2024 14:24
Copy link

vercel bot commented May 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 7, 2024 0:43am

@joshuaellis joshuaellis force-pushed the chore/update-ds-2.0.0-beta.3 branch from f06119b to dc1f8d4 Compare May 7, 2024 09:09
@joshuaellis joshuaellis changed the title chore: update styled-components to 6 & update DS to 2.0.0-beta.3 chore!: update styled-components to 6 & update DS to 2.0.0-beta.3 May 7, 2024
@joshuaellis joshuaellis requested a review from Bassel17 May 7, 2024 09:13
Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

Mostly just little things.

The only big thing is my concern about requiring everything to be wrapped in Fields with an explicit label, it feels like the increased verbosity will lead to more mistakes and a steeper learning curve. But this isn't my scope so I won't object too strongly 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a ton of unused imports in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we can ignore that, @madhurisandbhor is working on re-implementing it all in a different PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are a small handful of components that still use ariaLabel instead of aria-label, is that intentional? It feels inconsistent

Copy link
Member Author

Choose a reason for hiding this comment

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

Depends on the context but most likely yes, we typically pass prop as camelCase and convert to kebab-case. But also, we also have bad naming on some props...

@joshuaellis
Copy link
Member Author

Mostly just little things.

The only big thing is my concern about requiring everything to be wrapped in Fields with an explicit label, it feels like the increased verbosity will lead to more mistakes and a steeper learning curve. But this isn't my scope so I won't object too strongly 😆

Maybe, in an ideal world we'd all just use the form input renderer and we just have one set of fields to maintain, then there is no learning curve.

Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

LGTM!

QAd it and only one small issue now, the focus-visible state for text inputs shows the border on the actual inner text input rather than the outer element.

image

@joshuaellis
Copy link
Member Author

QAd it and only one small issue now, the focus-visible state for text inputs shows the border on the actual inner text input rather than the outer element.

I'll make this a bug in JIRA

@joshuaellis joshuaellis merged commit b7c6256 into v5/main May 7, 2024
75 of 84 checks passed
@joshuaellis joshuaellis deleted the chore/update-ds-2.0.0-beta.3 branch May 7, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: 💥 Breaking change This PR contains breaking changes and should not be merged pr: chore This PR contains chore tasks (cleanups, configs, tooling...) source: dependencies Source is dependency problem
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants