-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
.circleci/config.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing with plotly/dash-core-components#848.
const getTransform = (element, propName, propPart) => | ||
propPart | ||
? element.persistenceTransforms[propName][propPart] | ||
: noopTransform; |
There was a problem hiding this comment.
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() | |||
|
|||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dash-renderer/src/persistence.js
Outdated
? element.persistenceTransforms[propName][propPart] | ||
: noopTransform; | ||
const getTransform = (element, propName, propPart) => { | ||
if (typeof element.persistenceTransforms !== 'undefined') { |
There was a problem hiding this comment.
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:
if (typeof element.persistenceTransforms !== 'undefined') { | |
if (element.persistenceTransforms) { |
any truth-y value will pass the test (https://dorey.github.io/JavaScript-Equality-Table/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dash-renderer/src/persistence.js
Outdated
const getTransform = (element, propName, propPart) => { | ||
if (typeof element.persistenceTransforms !== 'undefined') { | ||
if ( | ||
Object.getOwnPropertyNames(element.persistenceTransforms).includes( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔪
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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):
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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):
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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>
.circleci/config.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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', | ||
], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dash-renderer/src/persistence.js
Outdated
? element.persistenceTransforms[propName][propPart] | ||
: noopTransform; | ||
const getTransform = (element, propName, propPart) => { | ||
if (element.persistenceTransforms) { |
There was a problem hiding this comment.
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]) {
Fixes plotly/dash-core-components#700.
The persistence logic in DatePickerRange and DatePickerSingle have been modified to extract/apply just the
date
portion of thedate-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
, butpersistence.js
has to be modified to check for transformation on bothpropPart
andpropName
.Contributor Checklist
PersistenceTransforms
PersistenceTransforms
https://github.com/plotly/dash-core-components/pull/848/files