-
Notifications
You must be signed in to change notification settings - Fork 24
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 rangeInput guidence and variants #1898
base: master
Are you sure you want to change the base?
Conversation
|
||
- The RangeInput should be clear within the label for the user to know what value they are choosing. | ||
- There should be the min and the max on each side of the RangeInput so the user is aware of these values. | ||
- Do not use ranges that are to large i.e 1-10000 or ranges that are very small i.e 1-3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I totally agree with this point. If I adjust my "step" appropriately, why would it matter how small/large my range is?
Trying to find our research to reference, but am not seeing it the Figma file.
- Do not use ranges that are to large i.e 1-10000 or ranges that are very small i.e 1-3 | |
- Avoid ranges that are too large (e.g. 1-10000) or ranges that are very small (e.g. 1-3). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't taking into account if you adjust your step
appropriately then it shouldn't matter. I was reading a article about designing slider
and it mentioned this but your right if the step
is adjusted accordingly it should be fine!
- There should be the min and the max on each side of the RangeInput so the user is aware of these values. | ||
- Do not use ranges that are to large i.e 1-10000 or ranges that are very small i.e 1-3 | ||
|
||
### Usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to hear about more tangible use cases / applications (e.g. volume, lighting dimmer, other more enterprise focussed use cases)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill add some more to Usage
for this!
return ( | ||
<FormField label="Label" help="RangeInput Description"> | ||
<Box | ||
pad={{ horizontal: '11px', vertical: '5px' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the custom pixel value padding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so with the inputs
we have this in the theme.. here
however, this is not applying to rangeinput
so that is why I have it here within the box
wrapping it.. I agree this is not ideal so I can look at how we can enhance grommet to also have some pad on rangeinput
otherwise when wrapping this in formfield
it looks horrible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely address this in grommet/theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree Ill peak around today!
aries-site/src/examples/components/rangeinput/RangeInputDescriptionExample.js
Outdated
Show resolved
Hide resolved
section, therefor h2 is the semantically correct heading. For | ||
additional detail, see https://design-system.hpe.design/foundation/typography#semantic-usage-of-heading-levels). */} | ||
<Heading level={2} margin="none"> | ||
Book a Hotel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should come up with a more "enterprise focused" / HPE relevant example. A recurring critique / ask of the Design System is to present more relevant examples to HPE use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good Ill chat with @vavalos5 about this so we can line up figma with another example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed @halocline i'll peek around other files to find where we might be using RangeInput
within a form
<Box direction="row" gap="medium" width="large"> | ||
<Box | ||
pad={{ horizontal: '11px', vertical: '5px' }} | ||
direction="row" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch I fixed that!
Co-authored-by: Matthew Glissmann <[email protected]>
Co-authored-by: Matthew Glissmann <[email protected]>
Co-authored-by: Matthew Glissmann <[email protected]>
…ptionExample.js Co-authored-by: Matthew Glissmann <[email protected]>
Im going to move this PR into Draft there is some things to discuss on how we can improve the RangeInput on Grommet. |
I made a new issue for the things that I was running into with this component |
For now going to align the new styles of the thumb with figma which is tracked here grommet/grommet-theme-hpe#212 |
What does this PR do?
This PR adds some variants to the
rangeInput
page as well as a couple more examples to match up with the figma filesWhere should the reviewer start?
rangeInput/
What testing has been done on this PR?
site
How should this be manually tested?
the examples on the site
Any background context you want to provide?
In figma there is a section that has
RangeInput
disabled however this is not achievable within Grommet today so if we want to add that I can open a new ticket to add propdisabled
to RangeInput in GrommetThere are some theme changes so we can take out the border on the thumb this is no longer needed as well as the
box shadow
this is going to align more how grommetrangeinput
works when you hoverWhat are the relevant issues?
closes #1892
Screenshots (if appropriate)
Should this PR be mentioned in Design System updates?
sure
Is this change backwards compatible or is it a breaking change?
backwards compatible