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

feat(FluidNumberInput): add FluidNumberInput variant #12737

Merged
merged 14 commits into from
Dec 13, 2022
Merged

feat(FluidNumberInput): add FluidNumberInput variant #12737

merged 14 commits into from
Dec 13, 2022

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Nov 28, 2022 •
edited
Loading

Refs #12126
Closes #12454
Closes #12455
Closes #12826

-Adds in the FluidNumberInput component as unstable

Changelog

New

  • unstable__FluidNumberInput component
  • Styles for FluidNumberInput (_fluid-number-input.scss)
  • Stories for FluidNumberInput (FluidNumberInput.stories.js)
  • Tests for FluidNumberInput

Changed

  • Updates index, snapshots

Testing / Reviewing

Go to FluidNumberInput and ensure all stories look correct

@netlify
Copy link

netlify bot commented Nov 29, 2022

❌ Deploy Preview for carbon-elements failed.

Name Link
🔨 Latest commit 1c984ba
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6386307198c6910008df36ad

@netlify
Copy link

netlify bot commented Nov 29, 2022 •
edited
Loading

✅ Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ac53674
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/63988a6fe4f90d00089b4ccb
😎 Deploy Preview https://deploy-preview-12737--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 29, 2022

✅ Deploy Preview for carbon-components-react ready!

Name Link
🔨 Latest commit 1c984ba
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63863071a594960009b16164
😎 Deploy Preview https://deploy-preview-12737--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Nov 29, 2022 •
edited
Loading

✅ Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ac53674
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63988a6fbbf3fd0009115ae8
😎 Deploy Preview https://deploy-preview-12737--carbon-components-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tw15egan tw15egan force-pushed the fluid-number-input branch 2 times, most recently from 77366d2 to 822f206 Compare November 30, 2022 20:42
@tw15egan tw15egan marked this pull request as ready for review December 8, 2022 16:10
@tw15egan tw15egan requested review from a team as code owners December 8, 2022 16:10
tay1orjones
tay1orjones requested changes Dec 8, 2022
View reviewed changes
Copy link
Member

@tay1orjones tay1orjones 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 super good! A couple minor things below.

Also, I think packages/styles/scss/components/__tests__/number-input-test.js (and others - fluid-combo-box, fluid-multiselect, etc.) need updated to ensure the new mixins exist. Sorry I didn't catch this before - might be something to do for all fluid inputs in a separate PR?

packages/react/src/components/FluidNumberInput/FluidNumberInput.stories.js Outdated Show resolved Hide resolved
packages/react/src/components/FluidNumberInput/FluidNumberInput.stories.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Member Author

tw15egan commented Dec 8, 2022

I'll tackle that in a separate PR (created issue #12817) if that's alright. Pushed up changes adding all skeleton components as subcomponents to the story files

aagonzales
aagonzales reviewed Dec 9, 2022
View reviewed changes
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

It looks good except for the label overflow (which I keep forgetting about). It needs to overflow like how we set it up in fluid text input. But that's the only thing I see wrong.

image

@tw15egan tw15egan mentioned this pull request Dec 9, 2022
Closed
10 tasks
@tw15egan
Copy link
Member Author

tw15egan commented Dec 9, 2022

@aagonzales I'll update this one here, and I've created a new issue (#12825) to track the rest of the components and will add the same styles to all of them in a separate PR

aagonzales
aagonzales reviewed Dec 9, 2022
View reviewed changes
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

That looks good! One more change after talking to Jeannie, we want to just use the regular $body-compact-01 here instead of the mono $code-01. She wants to change it in the fixed version as well.

alisonjoseph
alisonjoseph approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM!

@tw15egan
Copy link
Member Author

@aagonzales updated 👍🏻

@tw15egan tw15egan mentioned this pull request Dec 12, 2022
Closed
2 tasks
@aagonzales
Copy link
Member

@tw15egan it does look like the number are appearing in Plex.

image

What the plex number should look like
image

@tw15egan
Copy link
Member Author

@aagonzales good catch, forgot number inputs reset their own styles, and it was using the browser default. Updated it, so it should not load in IBM Plex Sans

aagonzales
aagonzales approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Perfect! Ship it!

@kodiakhq kodiakhq bot merged commit 5426879 into carbon-design-system:main Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonjoseph alisonjoseph alisonjoseph approved these changes

@tay1orjones tay1orjones tay1orjones approved these changes

@aagonzales aagonzales aagonzales approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
Design System
Archived in project
Milestone
No milestone
4 participants
@tw15egan @aagonzales @alisonjoseph @tay1orjones