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

Drop all es5 mentions and support #7229

Merged
merged 10 commits into from
Dec 30, 2019
Merged

Drop all es5 mentions and support #7229

merged 10 commits into from
Dec 30, 2019

Conversation

denis-anisimov
Copy link
Contributor

@denis-anisimov denis-anisimov commented Dec 23, 2019

Fixes #6856


This change is Reviewable

Denis Anisimov added 3 commits December 23, 2019 09:59
- Remove WebBrowser parameter from methods
- Correct tests and test data
- Update webpack config
}

private Element createInlineDependencyElement(VaadinSession session, VaadinRequest request, JsonObject dependency,
Dependency.Type type) {
private Element createInlineDependencyElement(VaadinSession session,
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove this unused method parameter "session". rule

case JAVASCRIPT:
dependencyElement = createJavaScriptElement(url, false, null);
break;
case JS_MODULE:
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Reduce this switch case number of lines from 6 to at most 5, for example by extracting code into methods. rule

null),
path -> service.resolveResource(path, browser), this);
path -> service.getResourceAsStream(path, null),
path -> service.resolveResource(path), this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Replace this lambda with a method reference. rule

null),
resourcePath -> resolveResource(resourcePath, browser),
resourcePath -> getResourceAsStream(resourcePath, null),
resourcePath -> resolveResource(resourcePath),
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Replace this lambda with a method reference. rule

@joheriks joheriks self-requested a review December 27, 2019 07:39
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from External Reviews to Changes Requested Dec 27, 2019
Copy link
Contributor

@joheriks joheriks left a comment

Choose a reason for hiding this comment

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

Reviewed 50 of 50 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/server/UnsupportedBrowserHandler.java, line 50 at r1 (raw file):

            + "      padding-top: 2em;" + CLOSING_BRACKET + "    sub,"
            + "    small {" + "      color: #999;" + CLOSING_BRACKET
            + "  </style>" + "</head>";

The readability got strictly worse by this auto-formatting. Perhaps leave the original formatting and surround the block with

 //@formatter:off
...
 //@formatter:on

to prevent automatic formatting?

Copy link
Contributor Author

@denis-anisimov denis-anisimov 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: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @joheriks)


flow-server/src/main/java/com/vaadin/flow/server/UnsupportedBrowserHandler.java, line 50 at r1 (raw file):

Previously, joheriks (Johannes Eriksson) wrote…

The readability got strictly worse by this auto-formatting. Perhaps leave the original formatting and surround the block with

 //@formatter:off
...
 //@formatter:on

to prevent automatic formatting?

Done.

Copy link
Contributor

@joheriks joheriks 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 1 of 1 files at r2.
Reviewable status: 4 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @denis-anisimov)

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Changes Requested to Reviewer approved Dec 30, 2019
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 11 issues

  • MAJOR 2 major
  • MINOR 8 minor
  • INFO 1 info

Watch the comments in this conversation to review them.

7 extra 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:

  1. MINOR BootstrapHandler.java#L556: Remove this use of "BootstrapPageResponse"; it is deprecated. rule
  2. MINOR BootstrapHandler.java#L556: Remove this use of "BootstrapPageResponse"; it is deprecated. rule
  3. MINOR BootstrapHandler.java#L560: Remove this use of "modifyBootstrapPage"; it is deprecated. rule
  4. MINOR VaadinService.java#L180: Remove this use of "BootstrapListener"; it is deprecated. rule
  5. MINOR VaadinService.java#L293: Remove this use of "getBootstrapListeners"; it is deprecated. rule
  6. MINOR VaadinService.java#L293: Remove this use of "getAddedBootstrapListeners"; it is deprecated. rule
  7. INFO VaadinService.java#L642: Do not forget to remove this deprecated code someday. rule

Copy link
Contributor Author

@denis-anisimov denis-anisimov 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 4 discussions.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@denis-anisimov denis-anisimov merged commit 92bffc0 into ccdm Dec 30, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Dec 30, 2019
@denis-anisimov denis-anisimov deleted the 6856-drop-es5 branch December 30, 2019 09:58
vlukashov pushed a commit to vaadin/flow-and-components-documentation that referenced this pull request Dec 30, 2019
This fixes the documentation CI build after the vaadin/flow#7229 PR was merged.
denis-anisimov pushed a commit to vaadin/flow-and-components-documentation that referenced this pull request Dec 31, 2019
This fixes the documentation CI build after the vaadin/flow#7229 PR was merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants