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

✨ RSP-631: Datepicker range styles added #13

Closed
wants to merge 6 commits into from
Closed

Conversation

GarthDB
Copy link
Member

@GarthDB GarthDB commented Nov 17, 2018

No description provided.

@GarthDB GarthDB changed the title ✨ RSP-631: Datepicker range styles added ✨ RSP-631: Datepicker range styles added Dec 6, 2018
hr {
background-color: var(--spectrum-textfield-placeholder-text-color);
border: 0;
margin: 15px 0px;
Copy link
Contributor

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

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Contributor

@NateBaldwinDesign NateBaldwinDesign left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor

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;
Copy link
Contributor

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*/
Copy link
Contributor

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;
Copy link
Contributor

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.
image

Copy link
Contributor

@majornista majornista Dec 14, 2018

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;
Copy link
Contributor

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;
Copy link
Contributor

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*/
Copy link
Contributor

Choose a reason for hiding this comment

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

should not use pixel values or it won't scale properly. Be sure to test locally by swapping themes and scales in order to see this in action (note the location of the


):

image

}
/** Quiet **/
&.spectrum-InputGroup--quiet {
border-bottom: 1px solid var(--spectrum-textfield-border-color);
Copy link
Contributor

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 ?

@GarthDB
Copy link
Member Author

GarthDB commented Jan 7, 2019

Possible duplicate of #53

@GarthDB
Copy link
Member Author

GarthDB commented Jan 7, 2019

Closing as the work has moved to #53

@GarthDB GarthDB closed this Jan 7, 2019
@GarthDB GarthDB deleted the end73241/RSP-631 branch September 30, 2021 20:39
@github-actions github-actions bot mentioned this pull request Apr 19, 2024
@github-actions github-actions bot mentioned this pull request Apr 26, 2024
@github-actions github-actions bot mentioned this pull request May 30, 2024
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.

3 participants