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

Refresh inputs #1029

Merged
merged 4 commits into from Feb 21, 2020
Merged

Refresh inputs #1029

merged 4 commits into from Feb 21, 2020

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Feb 20, 2020

This updates the inputs (mostly .form-control).

Screen Shot 2020-02-21 at 10 49 02 PM

👀 Preview

It's probably not the final version, but maybe ok to start testing on dotcom.

TODO

  • Update docs
  • Replace border color
  • Increase border radius
  • Increase padding (only on the sides, height doesn't change)
  • And a few more tweaks

/cc @primer/ds-core

@simurai simurai requested a review from emplums February 20, 2020 04:53
@vercel
Copy link

vercel bot commented Feb 20, 2020

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

🔍 Inspect: https://zeit.co/primer/primer-css/fc39qph72
✅ Preview: https://primer-css-git-next-inputs.primer.now.sh

@vercel vercel bot temporarily deployed to Preview February 20, 2020 04:56 Inactive
@vercel vercel bot temporarily deployed to Preview February 20, 2020 05:03 Inactive
@simurai simurai added this to 🚧 Work in Progress in 📦 Primer CSS release tracking via automation Feb 20, 2020
@simurai simurai moved this from 🚧 Work in Progress to 🔍 Ready to release in 📦 Primer CSS release tracking Feb 20, 2020
@colebemis
Copy link
Contributor

I'm not sure if this is the best place for me to leave this feedback but the x-axis padding seems like a little too much. IMO 8px or 12px on the left and right feels better to me than 16px (I know 12px isn't in our spacing scale 😬). I'm interested in hearing what other people think though 😺

image

@simurai
Copy link
Contributor Author

simurai commented Feb 20, 2020

IMO 8px or 12px on the left and right feels better to me than 16px (I know 12px isn't in our spacing scale 😬).

Yeah.. I guess the 16px in inputs is meant to match the buttons. Here all 3 variants:

8px 12px 16px
Screen Shot 2020-02-20 at 2 42 24 PM Screen Shot 2020-02-20 at 2 41 13 PM Screen Shot 2020-02-20 at 2 40 22 PM

I like the extra padding (16px) on the buttons. But if button and inputs should match, and we ignore our spacing scale 🤷‍♂, 12px feels the most balanced?

Edit: Hey, our linter doesn't complain if we use ($spacer-1 + $spacer-2) -> (4px + 8px). ✌️ 😜

@auareyou
Copy link
Contributor

I see what you mean @colebemis - Personally, I'd to formally add 4px, 12px and 20px as exceptions and document why we need those exceptions. But since we're not making spacing changes this time around I think the most sensitive would be going with 8px and see how it feels. If we have to revisit in a second iteration we can. I'll update the sticker sheet.

@auareyou
Copy link
Contributor

Sorry, I missed the examples. I do agree 12px feels the most balanced. If we can combine spacers then let's do it!

Copy link
Contributor

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

Let's switch to $spacer-1 + $spacer-2 for the forms padding 🙂

Copy link
Contributor

@emplums emplums left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@vercel vercel bot temporarily deployed to Preview February 21, 2020 01:05 Inactive
@simurai
Copy link
Contributor Author

simurai commented Feb 21, 2020

Ok, it's now changed to 12px padding on the sides.

Screen Shot 2020-02-21 at 10 04 06 AM

It's just hardcoded. Maybe we shouldn't try to trick the linter with ($spacer-1 + $spacer-2) so that using variables stays predictable. One day we might can look into switching to em based padding, like this.

Copy link
Contributor

@auareyou auareyou left a comment

Choose a reason for hiding this comment

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

🙌Yas!

📦 Primer CSS release tracking automation moved this from 🔍 Ready to release to ✅ Approved for release Feb 21, 2020
@simurai simurai merged commit 8f12463 into next Feb 21, 2020
📦 Primer CSS release tracking automation moved this from ✅ Approved for release to 💜 Done Feb 21, 2020
@simurai simurai deleted the next-inputs branch February 21, 2020 14:06
@simurai simurai mentioned this pull request Feb 24, 2020
34 tasks
@simurai simurai mentioned this pull request Jul 2, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants