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

feat: today actions for datepicker #964

Merged
merged 40 commits into from
Jul 6, 2020
Merged

Conversation

prsdthkr
Copy link
Member

@prsdthkr prsdthkr commented Apr 14, 2020

Description

This change adds the ability to configure a todayAction for the DatePicker.

For example, todayAction={{type: 'select', label: 'Today'}}

todayAction.type is a string indicating the type of today button.

  • 'none' (default) today button won't be shown
  • 'select' today button as footer action that selects today's date and closes datepicker
  • 'navigate' today button as header action that navigates (i.e. sets focus) to today's date

todayAction.label is a localized string label for the today action button.

The button will only be rendered if:

  • todayAction.type is 'select' or 'navigate'
  • AND todayAction.label is a valid non-empty string

Other changes:

  • Add compact mode for Calendar
  • Use calendar compact mode for compact Datepicker

DatePicker today actions

@netlify
Copy link

netlify bot commented Apr 14, 2020

Deploy preview for fundamental-react ready!

Built with commit f02fb69

https://deploy-preview-964--fundamental-react.netlify.app

@prsdthkr prsdthkr self-assigned this Apr 14, 2020
@prsdthkr prsdthkr marked this pull request as ready for review April 14, 2020 03:48
@prsdthkr prsdthkr requested review from a team April 14, 2020 03:49
# Conflicts:
#	src/DatePicker/DatePicker.js
# Conflicts:
#	src/Calendar/Calendar.js
#	src/DatePicker/DatePicker.Component.js
#	src/DatePicker/DatePicker.js
#	src/DatePicker/__stories__/DatePicker.stories.js
#	storybook-testing/__image_snapshots__/storyshots-test-js-storyshots-components-date-picker-today-button-1-snap.png
@prsdthkr prsdthkr requested review from jbadan and meganaconley June 15, 2020 21:56
@prsdthkr prsdthkr changed the title feat: today button for date picker feat: today selection footer button for datepicker Jun 15, 2020
@prsdthkr prsdthkr added DatePicker enhancement New feature or request labels Jun 15, 2020
@prsdthkr prsdthkr requested a review from bcullman June 16, 2020 21:40
@bcullman
Copy link
Contributor

should there be a boolean to enableTodaySelect here?

@prsdthkr prsdthkr removed the WIP Work in Progress label Jun 22, 2020
@jbadan
Copy link
Contributor

jbadan commented Jun 30, 2020

@prsdthkr are you waiting for a review on this or it is it progress?

@prsdthkr
Copy link
Member Author

prsdthkr commented Jul 1, 2020

@prsdthkr are you waiting for a review on this or it is it progress?

@jbadan awaiting review.

@@ -453,6 +453,7 @@ class Calendar extends Component {
this.props.localizedText.show12PreviousYears : this.props.localizedText.previousMonth;
const nextButtonLabel = this.state.showYears ?
this.props.localizedText.show12NextYears : this.props.localizedText.nextMonth;
const compact = this.props?.compact || false;
Copy link
Contributor

Choose a reason for hiding this comment

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

this.props should never be undefined and is this necessary or can we just use this.props.compact below?

if (!disableStyles && this._showTodayFooter()) {
//below styles are needed for footer styling
require('fundamental-styles/dist/dialog.css');
require('fundamental-styles/dist/bar.css');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the old way of including styles, now they're expected to be imported with the rest of the imports without conditionals.

@prsdthkr prsdthkr requested a review from skvale July 1, 2020 22:45
@@ -96,6 +98,7 @@ export const dev = () => (
dateKnobToDate('disable between dates (2)', disabledDateSecondDefault)]}
locale={text('locale', 'en')}
openToDate={dateKnobToDate('open to date', new Date())}
showToday={boolean('showToday', false)}
Copy link
Contributor

Choose a reason for hiding this comment

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

could add compact to as a knob to the dev story as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

const { enableRangeSelection, todayAction: { label: todayLabel, type: todayType } } = this.props;
return todayType === 'select'
&& !enableRangeSelection
&& isEnabledDate(moment(), this.props)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we not show the footer button if

&& !enableRangeSelection
&& isEnabledDate(moment(), this.props)

but we don't consider these things for the header button?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because clicking the footer today button selects today's date and closes the popover, I think this would not be a valid interaction if a date range is to be selected or if today's date is disabled. Also, since the header today button only performs navigation we can show it regardless. What do you think?

className,
{
'fd-calendar--compact': compact
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible className would override a css rule from rd-calendar--compact and it looks like the convention in fundamental-react is to keep the custom className last

        const calendarClasses = classnames(
            'fd-calendar',
            {
                'fd-calendar--compact': compact
            },
            className
        );

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

'fd-bar--cosy': !compact,
'fd-bar--compact': compact
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a prop so a consumer can add their own className to this? footerClassName?

@@ -309,6 +311,28 @@ class DatePicker extends Component {
this.validateDates(this.props.onBlur);
};

_showTodayHeader = () => {
Copy link
Contributor

@skvale skvale Jul 2, 2020

Choose a reason for hiding this comment

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

Any reason to prefix methods with _?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

@prsdthkr prsdthkr merged commit 9646fab into master Jul 6, 2020
@prsdthkr prsdthkr deleted the feat/date-picker-today-button branch July 7, 2020 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DatePicker enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants