feat: change version view modifiedOnly default to true
#11794
Conversation
GermanJablo
left a comment
GermanJablo
left a comment
There was a problem hiding this comment.
I made a small adjustment. Thanks!
philipp-tailor
left a comment
philipp-tailor
left a comment
There was a problem hiding this comment.
I made a small adjustment. Thanks!
@GermanJablo thank you!
| if (!modifiedOnly) { | ||
| current.delete('modifiedOnly') | ||
| if (modifiedOnly === false) { | ||
| current.set('modifiedOnly', 'false') |
There was a problem hiding this comment.
@GermanJablo Doesn't this change mean that every time the versions list is opened, there will be a client side routing adding the ?modifiedOnly=false query parameter?
IMHO query params should only indicate differences from the default state. Pushing URL changes trigger downstream effects like unnecessary route related computing and possibly re-rendering. That's why I didn't go for changing the URL query params in my initial commit.
There was a problem hiding this comment.
No, because modifiedOnly is now true by default. When you open the version list, it will be true, and therefore this condition won't be executed. To be sure, I've verified this with a console.log.
IMHO, query params should only indicate differences from the default state.
That's exactly what I'm doing now. Again, keep in mind that modifiedOnly is now true by default, so we only want to have false in the URL.
The conditions here are only for changing the state when the checkbox is checked or unchecked.
Your initial commit also had a bug where if you manually wrote modifiedOnly in the URL (I don't remember if it was with true, false, or both), the state wasn't reflected correctly.
|
this PR broke some tests. Can you fix that @philipp-tailor ? |
Replaces a more elaborate approach from payloadcms#11520 with the simplest solution, just changing the default. Co-authored-by: @GermanJablo
|
@GermanJablo I don't know how the earlier version with your commit did break the complete test suite, but I can confirm that it also didn't work for me locally, same as in CI. I re-created the same changes anew, and force pushed, which un-broke the test suite, uncovering 1 test, which I fixed in 385093d. Now there's still 1 E2E test that is failing. However that test is entirely unrelated to this code change. I hope it's possible for you to approve and merge given that the test failure is unrelated? |
GermanJablo
left a comment
GermanJablo
left a comment
There was a problem hiding this comment.
Thank you @philipp-tailor !
Replaces a more elaborate approach from #11520 with the simplest solution, just changing the default.
|
🚀 This is included in version v3.32.0 |
…s#11794) Replaces a more elaborate approach from payloadcms#11520 with the simplest solution, just changing the default.
2 participants
Replaces a more elaborate approach from #11520 with the simplest solution, just changing the default.