Store props from callback #1900
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
91a615d to
cdf9b8e
Compare
cdf9b8e to
bf208c4
Compare
alexcjohnson
reviewed
Mar 3, 2022
| @@ -306,6 +306,55 @@ const getProps = layout => { | ||
| }; | ||
| }; | ||
|
|
||
| export function setPersistance(layout, newProps, dispatch) { |
There was a problem hiding this comment.
@nickmelnikov82 my apologies for not reviewing this quicker!
Can you explain the differences between this new function and recordUiEdit? Ideally we'd 🌴 combine them into one (which we could call recordEdit I guess) with just a flag to distinguish the two situations, then any differences would be crystal clear.
The differences I see:
- Here you needed to create
finalPersistence- makes sense because the UI can't editpersistence- but in fact I don't see a harm in including this inrecordUiEdit, it's not a big overhead. Which brings up that in the callback case it's also possible to alterpersistence_type, so presumably we should also createfinalPersistenceType. - Why the short-circuit on
finalPersistence !== persistence? I guess maybe it's more complicated in this case, at least if you change from one truthypersistencevalue to another, but it seems to me like we'd still want the new value stored. - Can you explain the
storage.removeItemcase? Is that situation accessible from the UI as well (ie a case we were always missing)? recordUiEdithasconst vals = originalVal === undefined ? [newVal] : [newVal, originalVal];- ie we don't storeundefined. I'm not sure the practical consequences of this, but presumably we want to keep that behavior?
There was a problem hiding this comment.
- About finalPersistenceType it makes sense
- Yes, it's related to case when one truthy persistence value to another. When persistence is changed, the current value is immediately written to the storage as the original value.
- This is for the case described in the documentation
- Without these lines, more than half of the tests fail. I'm not quite sure how storage without the original value is used.
417b4fa to
747f63e
Compare
747f63e to
45c20af
Compare
45c20af to
c0c39b5
Compare
|
A related issue is the persistence doesn't work when a prop is set using query strings or path variables in a multi-page app. Here's a great example (thanks @nopria ) : https://github.com/nopria/dash-persistence-test Will this PR address this case as well? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As described in issue #1596 and #1856 props passed by callback are not saved in storage. This PR makes it possible to save props.
Contributor Checklist
optionals
CHANGELOG.md