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

chore: update style for core Select component #2058

Merged
merged 7 commits into from Aug 8, 2022
Merged

chore: update style for core Select component #2058

merged 7 commits into from Aug 8, 2022

Conversation

appsparkler
Copy link
Contributor

@appsparkler appsparkler commented Dec 30, 2021 •
edited by lachlanjc

Fixing the minor style issue in the core component so that we don't need to fix individual ones.

Release notes

Select component default paddingRight style has increased to spaces[4] (32px by default), to avoid text flowing behind the chevron icon.

@vercel
Copy link

vercel bot commented Dec 30, 2021 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/FtSW8HNYKNN9h9rhPLmrNqDpWQyr
✅ Preview: https://theme-ui-git-fork-appsparkler-patch-1-systemui.vercel.app

@appsparkler appsparkler mentioned this pull request Dec 30, 2021
Merged
@flo-sch flo-sch self-requested a review December 30, 2021 11:38
flo-sch
flo-sch reviewed Dec 30, 2021
View reviewed changes
Copy link
Collaborator

@flo-sch flo-sch left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

There is just a failing test in the CI, could you please update the snapshot?

packages/components/test/index.js

@hasparus
Copy link
Member

Okay, I don't have anything against this change, but are we really sure we want to add this 32px to all user projects?

I wouldn't classify this as breaking, but it might be annoying for some users.
It would need a release note, and I think we should bundle it with some more features.

@appsparkler
Copy link
Contributor Author

There is just a failing test in the CI, could you please update the snapshot?

@flo-sch , DONE : f596aaa

@flo-sch
Copy link
Collaborator

flo-sch commented Jan 3, 2022

I wouldn't classify this as breaking, but it might be annoying for some users.

It would need a release note, and I think we should bundle it with some more features.

Fair enough, it might indeed, especially when using another SVG arrow.

(The padding can still be customised from the theme, or?)

@hasparus hasparus added the enhancement New feature or request label Jan 3, 2022
@lachlanjc
Copy link
Member

Yeah, I do hesitate a bit with a "magic number", but at the same time, this has basically been a bug with our component if you're not making it full-width through some other means. E.g. with the theme on https://theme.hackclub.com, if I wasn't maximizing the width of the Select via CSS Grid columns, this is the default behavior:
image
We're already setting padding: 2, so setting paddingRight: 4 seems pretty reasonable to me. (Let's use 4 instead of 32 though, so it's customizable via the spacing scale.)

Appreciate this contribution!

@appsparkler
Copy link
Contributor Author

Let's use 4 instead of 32 though, so it's customizable via the spacing scale

Hi @lachlanjc; DONE : 14c58be

@hasparus hasparus added patch Increment the patch version when merged minor Increment the minor version when merged and removed patch Increment the patch version when merged labels Jan 13, 2022
@hasparus
Copy link
Member

hasparus commented Jan 13, 2022 •
edited

Hey @lachlanjc @appsparkler, I've added release notes section to the first post. Feel free to tweak it to your liking — it will land in the changelog and GitHub releases.

@lachlanjc I've also labeled this with minor as it may be surprising for some users, so how about we bundle it with removing Emotion styled (I promise I'll finish that PR soon 🤦‍♂️) and release as 0.14.0?

@lachlanjc lachlanjc mentioned this pull request Jan 13, 2022
Merged
@vercel
Copy link

vercel bot commented Jul 29, 2022 •
edited

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

Name Status Preview Updated
theme-ui ✅ Ready (Inspect) Visit Preview Jul 29, 2022 at 4:46AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 29, 2022 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit af3d5dc:

Sandbox Source
next-theme-ui-example Configuration
gatsby-plugin-theme-ui-example Configuration

lachlanjc
lachlanjc approved these changes Jul 31, 2022
View reviewed changes
Copy link
Member

@lachlanjc lachlanjc left a comment

Choose a reason for hiding this comment

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

@hasparus This is ready to merge!

@hasparus hasparus merged commit 19643f3 into system-ui:develop Aug 8, 2022
@appsparkler appsparkler deleted the patch-1 branch August 20, 2022 10:19
@hasparus
Copy link
Member

🚀 PR was released in v0.15.0 🚀

@hasparus hasparus added the released This issue/pull request has been released. label Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flo-sch flo-sch flo-sch approved these changes

@lachlanjc lachlanjc lachlanjc approved these changes

@hasparus hasparus Awaiting requested review from hasparus hasparus is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request minor Increment the minor version when merged released This issue/pull request has been released. @theme-ui/components
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@appsparkler @hasparus @flo-sch @lachlanjc