chore(Input): increment/decrement reset buttons focus stylings #3917
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
the
size:M
🦋 Changeset detectedLatest commit: be939a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
Status: Do Not Merge
github-actions
bot
added
the
Area: Components
☁️ Nx Cloud ReportCI is running/has finished running commands for commit be939a0. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
krisantrobus
commented
May 22, 2024
Comment on lines
35
to
37
| borderWidth="borderWidth10" | ||
| borderStyle="solid" | ||
| borderColor="transparent" |
There was a problem hiding this comment.
I had to add a border to the buttons and make it transparent. After changing the _focus style I saw the icons would move slightly due to the border changing for one and not the other. Issue shown in the video:
https://github.com/twilio-labs/paste/assets/55083528/c99f3ecd-1f9e-4b0b-93a0-98066daf7409
There was a problem hiding this comment.
ooh ok yeah i was wondering about the use of border colors here & how you got it to work.
This solution works just as well, but you could also use shadows instead. We have shadow border tokens (shadowBorderInverseStrong and shadowBorderPrimary) to account for this.
I have no like...eng perspective on which solution is better, so I'll rely on @nkrantz here for that.
There was a problem hiding this comment.
ok nvm I do have an opinion. Just realized it adds an extra 1px gray border all around the buttons.
todo: Let's use the shadow border tokens instead.
There was a problem hiding this comment.
Added boxShadow instead of border and that resolved all the spacing issues. Thank you! 🙇
|
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 be939a0:
|
|
Size Change: +45 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
Passing run #8059 ↗︎
Details:
Review all test suite changes for PR #3917 ↗︎ |
|||||||||||||||
krisantrobus
changed the title
nkrantz
reviewed
May 23, 2024
nkrantz
reviewed
May 23, 2024
| @@ -22,8 +24,17 @@ export const DecrementButton = React.forwardRef<HTMLButtonElement, DecrementButt | |||
| size="reset" | |||
| type="button" | |||
| borderRadius="borderRadius20" | |||
| backgroundColor="colorBackground" | |||
| backgroundColor={props.variant === "inverse" ? "colorBackgroundInverseStrong" : "colorBackground"} | |||
There was a problem hiding this comment.
nitpick: very much non-blocking but when using another prop, in most cases we add it to the list of destructured props on line 17 so that we can use variant rather than props.variant (especially if we're already destructuring other specific props in the component)
There was a problem hiding this comment.
Great suggestion 🙏 I'll make a change and push it up before we merge
nkrantz
reviewed
May 23, 2024
| @@ -713,6 +713,32 @@ export const DefaultNumberInput = (): React.ReactNode => { | |||
|
|
|||
| DefaultNumberInput.storyName = "Number Input - Controlled"; | |||
|
|
|||
| export const DefaultNumberInverseInput = (): React.ReactNode => { | |||
There was a problem hiding this comment.
praise: new story for this missed use case 👏
nkrantz
reviewed
May 23, 2024
There was a problem hiding this comment.
praise: great catch with the inverse variant!
suggestion(non-blocking): I would call this PR a chore rather than a feat
(This PR is great! I'm starting out by leaving way more comments and suggestions than is necessary just to give you more context about our code habits and standards. I don't mean to come across as nitpicky, feel free to ignore all the non-blocking comments)
krisantrobus
changed the title
serifluous
approved these changes
May 23, 2024
nkrantz
approved these changes
May 28, 2024
krisantrobus
added
the
🕵🏻♀️ Run website visual regression
Passing run #8061 ↗︎
Details:
Review all test suite changes for PR #3917 ↗︎ |
|||||||||||||||
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds correct focus stylings onto the Increment/Decrement buttons inside of Input component.
Accounts for inverse stylings of the buttons too.
Before:
After:
