-
Notifications
You must be signed in to change notification settings - Fork 198
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
✨ RSP-631: Datepicker range styles added #13
Conversation
✨ RSP-631: Datepicker range styles added
hr { | ||
background-color: var(--spectrum-textfield-placeholder-text-color); | ||
border: 0; | ||
margin: 15px 0px; |
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.
Do not use hard pixel values as they will not scale
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.
It would get converted to rems, but do you mean using the scaling tokens from dna?
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.
That’s correct it should be driven by DNA size tokens
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.
Some pixel values need to be replaced with global or alias tokens. I would like to see if we can make sure the --range
variant is flexible so that whether you have no width defined or a fixed width, the component looks balanced (placement of em dash).
border: 0; | ||
margin: 15px 0px; | ||
height: 1px; | ||
width: 10px; |
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.
@NateBaldwinDesign Should this be an actual em dash added with the :before pseudo-selector instead?
background-color: var(--spectrum-textfield-placeholder-text-color); | ||
border: 0; | ||
margin: 15px 0px; | ||
height: 1px; |
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.
use var(--spectrum-alias-border-size-thin)
width: 10px; | ||
position: absolute; | ||
top: 0; | ||
left: calc(50% - 27px); /*22px half button width and 5px hald of HR width*/ |
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.
also should be calculated using spectrum tokens rather than pixels so that it scales properly
/* Inputs */ | ||
.spectrum-InputGroup-field { | ||
width: 100px; | ||
min-width: 100px; |
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.
These widths don't seem appropriate. We would hope that the minimum width for each input would relate to the date format being used, which may not be able to be expressed here (fyi @GarthDB )
To that I would suggest leaving this out since textfields default min-width should already be set to 72px.
Ultimately we would hope that the spacing between the HR and the inputs always be the mid-way point, even if the field has a fixed width.
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.
@NateBaldwinDesign Adjusting the margin of the em dash based on the text width of the first input's value or placeholder requires a javascript method to execute when the value changes. Should we add a utility method to the docs to do this?
width: 100px; | ||
min-width: 100px; | ||
&:nth-of-type(1) { | ||
padding-right: 12px; |
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.
use var(--spectrum-global-dimension-size-150)
padding-right: 12px; | ||
} | ||
&:nth-of-type(2) { | ||
padding-left: 12px; |
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.
use var(--spectrum-global-dimension-size-150)
&.spectrum-InputGroup--quiet { | ||
border-bottom: 1px solid var(--spectrum-textfield-border-color); | ||
hr { | ||
left: calc(50% - 21px); /*22px half button width and 5px hald of HR width*/ |
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.
} | ||
/** Quiet **/ | ||
&.spectrum-InputGroup--quiet { | ||
border-bottom: 1px solid var(--spectrum-textfield-border-color); |
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.
Can't this be inherited from quiet textfields? @lazd ?
Possible duplicate of #53 |
Closing as the work has moved to #53 |
No description provided.