Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dp/ccdm/support inline bodysize viewport annotations #7168

Merged
merged 7 commits into from Dec 17, 2019

Conversation

ptdatkhtn
Copy link
Contributor

@ptdatkhtn ptdatkhtn commented Dec 15, 2019


This change is Reviewable

@ptdatkhtn ptdatkhtn added the hilla Issues related to Hilla label Dec 15, 2019
@ptdatkhtn ptdatkhtn self-assigned this Dec 15, 2019
@claassistantio
Copy link

claassistantio commented Dec 15, 2019

CLA assistant check
All committers have signed the CLA.

@claassistantio
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from External Reviews to Changes Requested Dec 16, 2019
Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ptdatkhtn)


flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinAppShellRegistry.java, line 182 at r1 (raw file):

        });

        getAnnotations(Viewport.class).forEach(viewport -> {

Viewport only should happen once, although compiler would fail when adding multiple annotations I don't know what happens if class is annotated, but also super class


flow-tests/test-ccdm/src/main/java/com/vaadin/flow/ccdmtest/AppShell.java, line 10 at r1 (raw file):

@Meta(name = "foo", content = "bar")
@PWA(name = "My App", shortName = "app")
@Viewport(Viewport.DEVICE_DIMENSIONS)

If you add this you need some assertions in the test class for checking that it happens.
See IndexHtmlRequestHandlerIT.indexHtmlRequestListener_openRootURL_shouldDynamicMetaContent

@caalador caalador removed this from Changes Requested in OLD Vaadin Flow ongoing work (Vaadin 10+) Dec 16, 2019
@ptdatkhtn
Copy link
Contributor Author


flow-tests/test-ccdm/src/main/java/com/vaadin/flow/ccdmtest/AppShell.java, line 10 at r1 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

If you add this you need some assertions in the test class for checking that it happens.
See IndexHtmlRequestHandlerIT.indexHtmlRequestListener_openRootURL_shouldDynamicMetaContent

Done.

@ptdatkhtn
Copy link
Contributor Author


flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinAppShellRegistry.java, line 182 at r1 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

Viewport only should happen once, although compiler would fail when adding multiple annotations I don't know what happens if class is annotated, but also super class

Done.

@ptdatkhtn
Copy link
Contributor Author


flow-server/src/main/java/com/vaadin/flow/server/startup/VaadinAppShellRegistry.java, line 188 at r2 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

MINOR Use isEmpty() to check whether the collection is empty or not. rule

Done.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 1 issue

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:

  1. MAJOR VaadinAppShellInitializer.java#L85: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed. rule

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 5 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @vaadin-bot)

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @vaadin-bot)

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @vaadin-bot from a discussion.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@manolo manolo merged commit 6af9655 into ccdm Dec 17, 2019
@manolo manolo deleted the dp/ccdm/support-inline-bodysize-viewport-annotations branch December 17, 2019 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hilla Issues related to Hilla +0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants