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

Add appearances and update inc/dec for number field #612

Merged
merged 14 commits into from
Jun 23, 2022

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Jun 21, 2022

Pull Request

🀨 Rationale

Fixes #361

πŸ‘©β€πŸ’» Implementation

  • Replaced inc/dec arrow glyphs with plus/minus
  • Added hover effect for inc/dec
  • Added appearance attribute and implemented missing outline and block appearances
  • Updated Storybook with new appearances

Note that the following are still incomplete/does not match the design doc:

  • Implement the invalid styling. This is not straightforward, since there isn't a slot for us to insert the exclamation icon.
  • Show the inc/dec buttons in disabled mode. The design doc says we should show them dimmed when disabled, but the FAST foundation number field's template hides them when disabled.

πŸ§ͺ Testing

Tested in Storybook

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@mollykreis
Copy link
Contributor

Similar to the combobox, I would have expected these "button" to be actual buttons with a pressed state & potentially to also be focusable. I'm guessing that is also difficult given the FAST template?

@m-akinc
Copy link
Contributor Author

m-akinc commented Jun 22, 2022

Similar to the combobox, I would have expected these "button" to be actual buttons with a pressed state & potentially to also be focusable. I'm guessing that is also difficult given the FAST template?

@mollykreis I think we can actually pass an html template (using Nimble buttons) for the "step-up" and "step-down" glyphs, so I think we could do what you suggest. Do we really want that? I'm not sure it's useful to have the inc/dec "buttons" be keyboard focusable, since you can do the same with the up/down arrows when the input itself is focused. Also, the inc/dec buttons are smaller than the standard button height/width, but I guess we could just size them down.

@mollykreis
Copy link
Contributor

Similar to the combobox, I would have expected these "button" to be actual buttons with a pressed state & potentially to also be focusable. I'm guessing that is also difficult given the FAST template?

@mollykreis I think we can actually pass an html template (using Nimble buttons) for the "step-up" and "step-down" glyphs, so I think we could do what you suggest. Do we really want that? I'm not sure it's useful to have the inc/dec "buttons" be keyboard focusable, since you can do the same with the up/down arrows when the input itself is focused. Also, the inc/dec buttons are smaller than the standard button height/width, but I guess we could just size them down.

@m-akinc, my thought is, yes, that is what we want, but I'm not necessarily the person to officially make that decision. Looking at the XD doc, though, it seems like Brandon used buttons within the component. They have a pressed state in XD. I think we'd want it to be consistent with the combobox, which also uses a smaller-than-standard button.

@m-akinc m-akinc requested review from rajsite and jattasNI June 22, 2022 20:35
@m-akinc m-akinc requested review from rajsite and mollykreis June 23, 2022 15:03
Copy link
Contributor

@mollykreis mollykreis left a comment

Choose a reason for hiding this comment

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

I agree with Milan's feedback and don't have anything additional to add.

@m-akinc m-akinc merged commit 2678f74 into main Jun 23, 2022
@m-akinc m-akinc deleted the users/makinc/numeric-styling branch June 23, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New design + block and outline appearances for nimble-number-field
4 participants