Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Use start date, if one is supplied, for initial_visible_month. #687

Merged
merged 9 commits into from
Nov 6, 2019

Conversation

shammamah-zz
Copy link
Contributor

Closes #679.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

Needs a changelog and some tdd with a test (jest is sufficient) showing that the value gets assigned correctly with the fix - a case for each value that overrides the default / is incorrect without the fix.

@@ -11,7 +11,9 @@ export default (newProps, momentProps) => {
dest[key] = null;

if (key === 'initial_visible_month') {
dest[key] = moment();
dest[key] = newProps['start_date'] || newProps['min_date_allowed'] ?
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Oct 28, 2019

Choose a reason for hiding this comment

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

From the PR:

the component is not responsive (nothing can be selected)

The selection is not actually disabled but as the initial month used to open the menu is after the end_date the dates are disabled -- moving back to a month before the end_date enables the menu. That said, this is a technicality, the behavior is unexpected and annoying especially if the date range is far in the future / past.

With the change here, the default behavior can also be confusig if end_date or max_date_allowed is set because they are not being used to decide the initial month displayed.

I think we could go further and also use end_date and max_date_allowed in determining the initial_visible_month if start_date and min_date_allowed are not available.

With

from datetime import datetime as dt

import dash
from dash.dependencies import Input, Output
import dash_core_components as dcc
import dash_html_components as html

app = dash.Dash('app',
    suppress_callback_exceptions=True)

app.layout = dcc.DatePickerRange(
    id='date-picker-range',
    # start_date = dt(2017, 1, 31),
    end_date = dt(2018, 3, 21),
    # min_date_allowed = dt(2017, 1, 31),
    max_date_allowed = dt(2018, 3, 21),
)

if __name__ == "__main__":
    app.run_server(debug=True)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far I have start_date -> min_date_allowed, and it makes the most sense to me to then look for end_date and max_date_allowed in that order if neither of those values are supplied. Do you think this is the best order to use?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. Makes sense to me :) low current -> lowest -> high current -> highest

Comment on lines 14 to 18
dest[key] = newProps['start_date'] || newProps['min_date_allowed']
|| newProps['end_date'] || newProps['max_date_allowed'] ?
moment(newProps['start_date'] || newProps['min_date_allowed']
|| newProps['end_date'] || newProps['max_date_allowed']) :
moment();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since moment() and moment(undefined) are equivalent (but not moment(null) 🙄), this can be simplified to:

dest[key] = moment(newProps['start_date'] || newProps['min_date_allowed'] || newProps['end_date'] || newProps['max_date_allowed'] || undefined);

@Marc-Andre-Rivet
Copy link
Contributor

@shammamah One small suggested code change and npm run format to be run for the linter to stop complaining :)

@shammamah-zz shammamah-zz force-pushed the 679-initial-visible-month branch 2 times, most recently from d15d912 to c1bbde2 Compare November 6, 2019 15:53
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateRangePicker: unable to select when initial_visible_month is not given
2 participants