Polish date-picker component #20824
Polish date-picker component #20824
Conversation
jasmussen
requested review from
ajitbohra,
chrisvanpatten and
jorgefilipecosta
as code owners
| @@ -7,6 +7,10 @@ | |||
| .components-datetime { | |||
| padding: $grid-unit-20; | |||
|
|
|||
| .components-popover__content & { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
|
This is the "before":
|
|
Size Change: -119 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
| 0 0 0 3px $blue-medium-focus; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What's all this red about :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The border radii for the am/pm buttons are even replaced by first-child/last-child rules in the buttongroup.
| @@ -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"> | |||
There was a problem hiding this comment.
Oh that's the red thing. Cool to reuse components. 👍
|
Thanks for the review. I see some test failures I have to update. |
|
I just tested... Clicking the close text brings one back to the calendar.
|
|
Indeed, excellent catch. Mind opening a new ticket for that? I don't think it's a result of this PR specifically, but feel free to point to it. |
This PR does a few things:
Screens: