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

PersistenceTransforms for date in datePickerRange and datePickerSingle #1376

Merged
merged 40 commits into from
Sep 4, 2020

Conversation

harryturr
Copy link
Contributor

@harryturr harryturr commented Aug 21, 2020

Fixes plotly/dash-core-components#700.

The persistence logic in DatePickerRange and DatePickerSingle have been modified to extract/apply just the date portion of the date-time. This will provide stability to date pickers (eg defined in callbacks) for the entire day they are defined.

The date logic is handled in plotly/dash-core-components#854 by using the class property PersistenceTransforms, but persistence.js has to be modified to check for transformation on both propPart and propName.

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@@ -104,7 +104,7 @@ jobs:
command: |
. venv/bin/activate && pip install --no-cache-dir --upgrade -e . --progress-bar off && mkdir packages
cd dash-renderer && renderer build && python setup.py sdist && mv dist/* ../packages/ && cd ..
git clone --depth 1 https://github.com/plotly/dash-core-components.git
git clone -b persistence-hg --depth 1 https://github.com/plotly/dash-core-components.git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

const getTransform = (element, propName, propPart) =>
propPart
? element.persistenceTransforms[propName][propPart]
: noopTransform;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old logic checks for just propPart for persistenceTransforms, but here we add logic to check if either propName or propPart is the persisted prop.

@@ -526,3 +532,189 @@ def set_out(val):
dash_duo.wait_for_text_to_equal(".out", "")

dash_duo.find_element("#btn").click()


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test persistence for DatePickerSingle and DatePicker range in callbacks, in conjunction with plotly/dash-core-components#848.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I think it's good to have at least one test that checks that persistence works as expected with and without a part, the tests validating the behavior of DPSingle and DPRange should be moved to DCC.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could create a new test component similar to https://github.com/plotly/dash/tree/dev/%40plotly/dash-generator-test-component-standard that has both (1) prop part, (2) no prop part for two different persisted props, and we can validate the behavior against that component instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harryturr harryturr marked this pull request as ready for review August 25, 2020 12:53
@harryturr harryturr changed the title Persistence for propName and propPart PersistenceTransforms for date in datePickerRange and datePickerSingle Aug 25, 2020
? element.persistenceTransforms[propName][propPart]
: noopTransform;
const getTransform = (element, propName, propPart) => {
if (typeof element.persistenceTransforms !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you can simplify the top level check to:

Suggested change
if (typeof element.persistenceTransforms !== 'undefined') {
if (element.persistenceTransforms) {

any truth-y value will pass the test (https://dorey.github.io/JavaScript-Equality-Table/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const getTransform = (element, propName, propPart) => {
if (typeof element.persistenceTransforms !== 'undefined') {
if (
Object.getOwnPropertyNames(element.persistenceTransforms).includes(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified to if(element.persistenceTransforms[propName]) in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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


@app.callback(Output("dps1-p", "children"), [Input("dps1", "date")])
def display_dps1(value):
print(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

🔪

Copy link
Contributor Author

Choose a reason for hiding this comment

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


@app.callback(Output("dpr1-p-start", "children"), [Input("dpr1", "start_date")])
def display_dps1(value):
print(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

🔪

Copy link
Contributor Author

Choose a reason for hiding this comment

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


@app.callback(Output("dpr1-p-end", "children"), [Input("dpr1", "end_date")])
def display_dps1(value):
print(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

🔪

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.key_down(Keys.SHIFT)
.send_keys(Keys.HOME)
.key_up(Keys.SHIFT)
.send_keys("01/02/2020")
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Aug 26, 2020

Choose a reason for hiding this comment

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

DRY 🌴🐫

def set_date_range(target, start_date, end_date):
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also dash_duo.clear_input that would simplify some of these (assuming there isn't some reason it fails for datepickers)

dash_duo.clear_input("#persistence-val")

.key_up(Keys.SHIFT)
.send_keys(Keys.DELETE)
.send_keys("01/01/2020")
.send_keys(Keys.ENTER)
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Aug 26, 2020

Choose a reason for hiding this comment

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

DRY 🌴🐫

def set_date(target, date):
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Apart from code comments, behavior seems fine.

Co-authored-by: Marc-André Rivet <Marc-Andre-Rivet@users.noreply.github.com>
@@ -104,7 +104,7 @@ jobs:
command: |
. venv/bin/activate && pip install --no-cache-dir --upgrade -e . --progress-bar off && mkdir packages
cd dash-renderer && renderer build && python setup.py sdist && mv dist/* ../packages/ && cd ..
git clone --depth 1 https://github.com/plotly/dash-core-components.git
git clone -b hg-700-persistence --depth 1 https://github.com/plotly/dash-core-components.git
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be unnecessary now that we test against a custom-made component

isNil(valueAsNumber) ? value : valueAsNumber,
nextProps.value
);
if (this.props.type !== 'number') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Stuff like number support can be 🔪 from the test component

'selectionStart',
'setProps',
'loading_state',
],
Copy link
Contributor

Choose a reason for hiding this comment

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

As we had discussed, this is copied over from dcc.Input. A lot of these props are not really useful for the test either. Try and keep the surface area for this component to the minimum you need to do the tests you need, nothing more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

? element.persistenceTransforms[propName][propPart]
: noopTransform;
const getTransform = (element, propName, propPart) => {
if (element.persistenceTransforms) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since both

    if (element.persistenceTransforms) {
        if (element.persistenceTransforms[propName]) {

result in return noopTransform, this can be simplified to a single if`

With the newest conditional chaining syntax this would become:

if (element.persistenceTransforms?.[propName]) { ... }

can also be be written with the more habitual

    if (element.persistenceTransforms && element.persistenceTransforms[propName]) {

@harryturr harryturr merged commit b6a1197 into dev Sep 4, 2020
@harryturr harryturr deleted the persistence-hg branch September 4, 2020 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dcc.DatePickerSingle() persistence not working
3 participants