fix(picker-column): dynamically change options #27359
Merged
Conversation
This file contains 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
|
|
amandaejohnston
requested changes
May 4, 2023
liamdebeasi
requested changes
May 4, 2023
amandaejohnston
requested changes
May 8, 2023
amandaejohnston
approved these changes
May 9, 2023
core/src/components/picker-column/test/dynamic-options/index.html
Outdated
Show resolved
Hide resolved
liamdebeasi
reviewed
May 9, 2023
core/src/components/picker-column/test/dynamic-options/picker-column.e2e.ts
Outdated
Show resolved
Hide resolved
core/src/components/picker-column/test/dynamic-options/picker-column.e2e.ts
Outdated
Show resolved
Hide resolved
liamdebeasi
reviewed
May 9, 2023
liamdebeasi
reviewed
May 10, 2023
liamdebeasi
requested changes
May 10, 2023
core/src/components/picker-column/test/dynamic-options/picker-column.e2e.ts
Outdated
Show resolved
Hide resolved
core/src/components/picker-column/test/dynamic-options/picker-column.e2e.ts
Outdated
Show resolved
Hide resolved
liamdebeasi
reviewed
May 10, 2023
|
|
||
| this.refresh(); | ||
| componentDidUpdate() { | ||
| // Options may have changed since last update. |
There was a problem hiding this comment.
Suggested change
| // Options may have changed since last update. | |
| // Options have changed since last update. |
If colDidChange is true then the columns definitely changed since the last render
liamdebeasi
approved these changes
May 10, 2023
| // Options may have changed since last update. | ||
| if (this.colDidChange) { | ||
| // Animation must be disabled through the `onDomChange` parameter. | ||
| // Otherwise, the recently added options will will render |
There was a problem hiding this comment.
Suggested change
| // Otherwise, the recently added options will will render | |
| // Otherwise, the recently added options will render |
| @@ -74,21 +80,32 @@ export class PickerColumnCmp implements ComponentInterface { | |||
| onEnd: (ev) => this.onEnd(ev), | |||
| }); | |||
| this.gesture.enable(); | |||
| // Options have not been initialized yet | |||
| // Animation must be disabled through the `noAnimate` flag | |||
| // Otherwise, the options will will render | |||
There was a problem hiding this comment.
Suggested change
| // Otherwise, the options will will render | |
| // Otherwise, the options will render |
| @@ -33,6 +33,12 @@ export class PickerColumnCmp implements ComponentInterface { | |||
| private rafId?: ReturnType<typeof requestAnimationFrame>; | |||
| private tmrId?: ReturnType<typeof setTimeout>; | |||
| private noAnimate = true; | |||
| // `colDidChange` is a flag that gets set when the column is changed | |||
| // dynamically. When this flag is set, the column will refresh | |||
| // to incorporate the new column data. | |||
There was a problem hiding this comment.
Suggested change
| // to incorporate the new column data. | |
| // after the component re-renders to incorporate the new column data. | |
| // This is necessary because `this.refresh` queries for the option elements, | |
| // so it needs to wait for the latest elements to be available in the DOM. |
| @@ -132,6 +149,7 @@ export class PickerColumnCmp implements ComponentInterface { | |||
| const scaleStr = `scale(${this.scaleFactor})`; | |||
|
|
|||
| const children = this.optsEl.children; | |||
|
|
|||
| @@ -185,6 +203,7 @@ export class PickerColumnCmp implements ComponentInterface { | |||
| button.classList.remove(PICKER_OPT_SELECTED); | |||
| } | |||
| } | |||
|
|
|||
amandaejohnston
approved these changes
May 11, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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.
Issue number: resolves #21763
What is the current behavior?
Picker options do not render correctly when dynamically changed. It throws an error when the original options' length is no longer the same as the updated options' length. This is due to
refresh()being called withthis.optsElhaving a stale list of children. The list doesn't get updated until the upcoming render.What is the new behavior?
The column will call
refresh()when it detects the columns has changed. The call needs to be done afterthis.optsElgets updated with the correct number of children.componentShouldUpdate()will check if options has changed -> re-renders with the updated options ->componentDidUpdate()callsrefresh()based oncomponentShouldUpdate()The standalone test page has been updated to include a way to test this fix.
Does this introduce a breaking change?
Other information
It might be beneficial to review the component lifecycle.
Co-authored-by: liamdebeasi liamdebeasi@users.noreply.github.com