Update renderer innerHTML on each attach to work properly with @PreserveOnRefresh #128
Conversation
|
|
|
SonarQube analysis reported 3 issues Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)
vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java, line 67 at r1 (raw file):
getElement().getNode().addAttachListener(this::attachComponentRenderer);
The previous code allows to do it only once and you didn't have to care about several attach events.
Now you adding renderer in the attach listener.
And it's never removed.
It means that if dialog is reattached to the same UI then this code will be executed several times.
Is it safe ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @denis-anisimov)
vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java, line 67 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
getElement().getNode().addAttachListener(this::attachComponentRenderer);The previous code allows to do it only once and you didn't have to care about several attach events.
Now you adding renderer in the attach listener.
And it's never removed.
It means that if dialog is reattached to the sameUIthen this code will be executed several times.Is it safe ?
To my understanding, the method attachComponentRenderer doesn't actually add something, it only updates the innerHTML of the renderer and so, for the same UI, should be an idempotent operation.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java, line 67 at r1 (raw file):
Previously, joheriks (Johannes Eriksson) wrote…
To my understanding, the method
attachComponentRendererdoesn't actually add something, it only updates theinnerHTMLof the renderer and so, for the same UI, should be an idempotent operation.
True.
There was a problem hiding this comment.
Reviewable status:
When a route leads to a component annotated with @PreserveOnRefresh, all direct UI children (such as dialogs and notifications) are transferred to the new UI (see vaadin/flow#5608). This requires the dialog to update the renderer on attach.
This change is