CCDM: support @Push annotation #7239
Conversation
project-bot
bot
added this to External Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
platosha
force-pushed
the
app/ccdm/feat-appshell-push
branch
from
f2681a4
to
913de31
Compare
caalador
removed this from External Reviews
in OLD Vaadin Flow ongoing work (Vaadin 10+)
platosha
force-pushed
the
app/ccdm/feat-appshell-push
branch
from
913de31
to
aaa0983
Compare
platosha
changed the title
There was a problem hiding this comment.
couple of tests broken and also needs rebasing
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @platosha)
flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 185 at r1 (raw file):
if (typeof pushScript === 'string') { await this.loadScript(pushScript);
this should be tested with intern
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java, line 273 at r1 (raw file):
public void modifyPushConfiguration(PushConfiguration pushConfiguration) { List<Push> pushAnnotations = getAnnotations(Push.class); if (pushAnnotations.size() > 1) {
This is never happening since the app shell class does not compiles with two @Push
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java, line 280 at r1 (raw file):
Push push = pushAnnotations.get(0); pushConfiguration.setPushMode(push.value()); pushConfiguration.setTransport(push.transport());
- if app-shell does not have any push configuration, it means that we take configuration for the current UI view, right?
- what happens when navigating to a new view with different push configuration, is that valid?
- does flow allow different
@Pushin multiple views with different properties ?
Fixes #6972
platosha
force-pushed
the
app/ccdm/feat-appshell-push
branch
from
aaa0983
to
c761d30
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @manolo and @platosha)
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java, line 273 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
This is never happening since the app shell class does not compiles with two
@Push
Copied after @BodySize, @Viewport, @PageTitle, they also have same checks and not repeatable.
Not sure, should we not check only for more than one @Push, but still check the other non-repeatable annotations?
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java, line 280 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
- if app-shell does not have any push configuration, it means that we take configuration for the current UI view, right?
- what happens when navigating to a new view with different push configuration, is that valid?
- does flow allow different
@Pushin multiple views with different properties ?
- yes
- AFAIK
@Pushwas not handled during navigation as of v14, so I didn’t introduce that handling either - AFAIK this is allowed, but only the one from initial view has effect
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @platosha)
flow-client/src/main/resources/META-INF/resources/frontend/Flow.ts, line 185 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
this should be tested with intern
It can be done in separated PR
flow-server/src/main/java/com/vaadin/flow/server/AppShellRegistry.java, line 273 at r1 (raw file):
Previously, platosha (Anton Platonov) wrote…
Copied after
@BodySize,@Viewport,@PageTitle, they also have same checks and not repeatable.Not sure, should we not check only for more than one
@Push, but still check the other non-repeatable annotations?
ok, let remove in a separated PR
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, 1 of 1 LGTMs obtained
| @@ -263,6 +265,26 @@ public void modifyIndexHtml(Document document, VaadinRequest request) { | |||
| document.body()::appendChild)); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
|
SonarQube analysis reported 1 issue
Watch the comments in this conversation to review them. |
There was a problem hiding this comment.
Dismissed @vaadin-bot from a discussion.
Reviewable status:
There was a problem hiding this comment.
Dismissed @vaadin-bot from a discussion.
Reviewable status:
Fixes #6972
This change is