Skip to content

Conversation

@srambach
Copy link
Member

@srambach srambach commented Feb 9, 2024

Fixes #5734

Adds tokens
Changes the date select to a menu-toggle

@patternfly-build
Copy link

patternfly-build commented Feb 9, 2024

@srambach srambach linked an issue Feb 9, 2024 that may be closed by this pull request
@srambach srambach requested review from a team, andrew-ronaldson, lboehling, mattnolting, mcoker and thatblindgeye and removed request for a team February 9, 2024 20:49
Copy link
Collaborator

@andrew-ronaldson andrew-ronaldson left a comment

Choose a reason for hiding this comment

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

Appppppproooooooooved

Copy link

@lboehling lboehling left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM! Couple of comments, I wonder if they're worth making either now or as follow ups:

I think you can remove this? It doesn't appear to do anything:

--#{$calendar-month}__dates-cell--m-selected__date--focus--BoxShadow: 0 0 #{pf-size-prem(5px)} var(--pf-t--global--background--color--primary--default);


If an item is selected and hovered, the light blue hover border is still showing

Screenshot 2024-02-12 at 8 55 31 PM

And if it's focused and hovered:

Screenshot 2024-02-12 at 8 55 53 PM

I think you can fix that by adding --#{$calendar-month}__date--hover--BorderWidth: 0; to the selected block vars

&.pf-m-selected {
--#{$calendar-month}__date--BackgroundColor: var(--#{$calendar-month}__dates-cell--m-selected__date--BackgroundColor);
--#{$calendar-month}__date--Color: var(--#{$calendar-month}__dates-cell--m-selected__date--Color);
--#{$calendar-month}__date--hover--BackgroundColor: var(--#{$calendar-month}__dates-cell--m-selected__date--hover--BackgroundColor);
--#{$calendar-month}__date--focus--BackgroundColor: var(--#{$calendar-month}__dates-cell--m-selected__date--focus--BackgroundColor);
--#{$calendar-month}__date--focus--after--BorderColor: var(--#{$calendar-month}__dates-cell--m-selected__date--focus--after--BorderColor);
--#{$calendar-month}__date--focus--BoxShadow: var(--#{$calendar-month}__dates-cell--m-selected__date--focus--BoxShadow);
--#{$calendar-month}__date--after--OutlineOffset: var(--#{$calendar-month}__date--after--focus--OutlineOffset);
}

Then it looks like this:

Hover:
Screenshot 2024-02-12 at 8 58 03 PM

Focus + hover:
Screenshot 2024-02-12 at 8 58 23 PM


The year input is cut off - can see it in the screenshots above

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Another thing that could be a followup is adding an icon to the helper text in the "Invalid" example

// @debug $date-picker; // check your variable names located in src/patternfly/sass-utilities/component-namespaces.scss

.#{$date-picker} {
:root, .#{$date-picker} {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a followup since we don't show examples with an input label like Figma seems to show ("Date of birth" above the datepicker), but we should add a var for spacing between a datepicker text label and the input itself (would also need to add an example).

@srambach
Copy link
Member Author

@lboehling @andrew-ronaldson I just want to check, can you confirm I should take off the hover border for the selected date?

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

Nice!! Just one small nit on the date picker selector


:root, .#{$date-picker} {
:where(:root),
.#{$date-picker} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.#{$date-picker} {
:where(.#{$date-picker}) {

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Preview doesn't look to have updated, but ran your branch locally and looks good

@mcoker mcoker merged commit 3794db9 into patternfly:v6 Feb 13, 2024
@patternfly-build
Copy link

🎉 This PR is included in version 6.0.0-alpha.80 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consume tokens: Calendar month and Date picker

6 participants