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

Polish date-picker component #20824

Merged
merged 5 commits into from
Mar 12, 2020
Merged

Polish date-picker component #20824

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
97468fb
Refine the buttongroup.
jasmussen Mar 12, 2020
030430f
Refactor am/pm picker to be buttongroup.
jasmussen Mar 12, 2020
8efc8ef
Polish padding.
jasmussen Mar 12, 2020
c7302b7
Try alternate fix.
jasmussen Mar 12, 2020
3503d1a
Fix to pass test, and update snaps.
jasmussen Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions packages/components/src/button-group/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
.components-button-group {
display: inline-block;
border-radius: $radius-block-ui;
border: $border-width solid $theme-color;

.components-button {
border-radius: 0;
display: inline-flex;
color: $theme-color;
box-shadow: inset 0 0 0 $border-width $theme-color;

& + .components-button {
margin-left: -1px;
}

&:first-child {
border-radius: $radius-block-ui 0 0 $radius-block-ui;
}

&:last-child {
border-radius: 0 $radius-block-ui $radius-block-ui 0;
}

// The focused button should be elevated so the focus ring isn't cropped,
// as should the active button, because it has a different border color.
&:focus,
Expand All @@ -22,7 +29,7 @@

// The active button should look pressed.
&.is-primary {
box-shadow: none;
box-shadow: inset 0 0 0 $border-width $theme-color;
}
}
}
48 changes: 8 additions & 40 deletions packages/components/src/date-time/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@
.components-datetime {
padding: $grid-unit-20;

.components-popover__content & {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recall having a conversation about keeping the padding behavior isolated to the context. But this feels a decent way to tune the behavior. Correct me if I'm wrong, Riad.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well ideally the component works the same in a popover or outside a popover. This is to say it shouldn't have padding IMO but I do remember the same discussion previously and you said that the calendar styles (third-party) kind of forces us to do have that padding somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that was the case due to how the month pagination works. However right at this moment I can't reproduce. Let me try a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue we had back then does not appear to be an issue anymore, so I've pushed a different fix in c7302b7

And things look right:

Screenshot 2020-03-12 at 12 02 13

Screenshot 2020-03-12 at 12 02 04

padding: 0;
}

.components-datetime__calendar-help {
padding: $grid-unit-20;

Expand Down Expand Up @@ -118,51 +122,15 @@
color: $dark-gray-500;
}

.components-datetime__time-am-button {
margin-left: 8px;
margin-right: -1px;
border-radius: 3px 0 0 3px;
}

.components-datetime__time-pm-button {
margin-left: -1px;
border-radius: 0 3px 3px 0;
}

.components-datetime__time-am-button:focus,
.components-datetime__time-pm-button:focus {
position: relative;
z-index: 1;
}

.components-datetime__time-am-button.is-pressed,
.components-datetime__time-pm-button.is-pressed {
background: $light-gray-300;
border-color: $dark-gray-100;
box-shadow: inset 0 2px 5px -3px $dark-gray-500;
&:focus {
box-shadow:
inset 0 2px 5px -3px $dark-gray-500,
0 0 0 1px $white,
0 0 0 3px $blue-medium-focus;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What's all this red about :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All that red duplicated the rules from the ButtonGroup component. In all my testing they are unnecessary with the refactor to use the actual ButtonGroup (and to toggle the selected button to primary in the ternery). You can recognize the margin-left: -1px from the button group, where it's used to ensure their borders stack. The z-index to ensure focus is uncropped is also there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The border radii for the am/pm buttons are even replaced by first-child/last-child rules in the buttongroup.

.components-datetime__time-field {

&-time {
/*rtl:ignore*/
direction: ltr;
}


&.am-pm button {
font-size: 11px;
font-weight: 600;
}

select {
margin-right: 4px;
margin-right: $grid-unit-05;

&:focus {
position: relative;
Expand All @@ -172,7 +140,7 @@

input[type="number"] {
padding: 2px;
margin-right: 4px;
margin-right: $grid-unit-05;
text-align: center;
-moz-appearance: textfield;

Expand All @@ -193,7 +161,7 @@
// We are assuming MM-DD-YYY correlates with AM/PM
&.is-12-hour {
.components-datetime__time-field-day input {
margin: 0 -4px 0 0 !important;
margin: 0 -$grid-unit-05 0 0 !important;
border-radius: $radius-round-rectangle 0 0 $radius-round-rectangle !important;
}
.components-datetime__time-field-year input {
Expand Down Expand Up @@ -230,5 +198,5 @@
// Hack to center the datepicker component within the popover.
// It sets its own styles so centering is tricky.
.components-popover .components-datetime__date {
padding-left: 4px;
padding-left: $grid-unit-05;
}
15 changes: 7 additions & 8 deletions packages/components/src/date-time/time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import Button from '../button';
import ButtonGroup from '../button-group';

/**
* Module Constants
Expand Down Expand Up @@ -318,24 +319,22 @@ class TimePicker extends Component {
/>
</div>
{ is12Hour && (
<div className="components-datetime__time-field components-datetime__time-field-am-pm">
<ButtonGroup className="components-datetime__time-field components-datetime__time-field-am-pm">
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's the red thing. Cool to reuse components. 👍

<Button
isSecondary
className="components-datetime__time-am-button"
isPressed={ am === 'AM' }
isPrimary={ am === 'AM' }
isSecondary={ am !== 'AM' }
onClick={ this.updateAmPm( 'AM' ) }
>
{ __( 'AM' ) }
</Button>
<Button
isSecondary
className="components-datetime__time-pm-button"
isPressed={ am === 'PM' }
isPrimary={ am === 'PM' }
isSecondary={ am !== 'PM' }
onClick={ this.updateAmPm( 'PM' ) }
>
{ __( 'PM' ) }
</Button>
</div>
</ButtonGroup>
) }
</div>
</fieldset>
Expand Down