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

Have a dedicated WrapMode for InitialPageSettings #3249

Merged
merged 4 commits into from
Jan 3, 2018
Merged

Conversation

caalador
Copy link
Contributor

@caalador caalador commented Jan 3, 2018

Also Fixes #3168


This change is Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Review status: 0 of 8 files reviewed at latest revision, 1 unresolved discussion.


flow-server/src/main/java/com/vaadin/flow/component/page/Inline.java, line 46 at r1 (raw file):

     * File content wrapping enum.
     */
    enum Wrapping {

Do we need both Inline.Wrapping and InitialPageSettings.WrapMode or could it be simplified?


Comments from Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


flow-documentation/application-structure/tutorial-bootstrap.asciidoc, line 115 at r1 (raw file):

    public void configurePage(InitialPageSettings settings) {
        settings.addInlineFromFile(InitialPageSettings.Position.PREPEND,
                "inline.js", InitialPageSettings.Wrapping.JAVASCRIPT);

Wrapping used here, WrapMode in the corresponding java file.


Comments from Reviewable

@caalador
Copy link
Contributor Author

caalador commented Jan 3, 2018

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


flow-documentation/application-structure/tutorial-bootstrap.asciidoc, line 115 at r1 (raw file):

WrapMode
My bad. fixed.


flow-server/src/main/java/com/vaadin/flow/component/page/Inline.java, line 46 at r1 (raw file):

Previously, ahie (Aleksi Hietanen) wrote…

Do we need both Inline.Wrapping and InitialPageSettings.WrapMode or could it be simplified?

The thing was that Inline has the AUTOMATIC mode that we can't use for inline content in InitialPageSettings


Comments from Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


flow-documentation/application-structure/tutorial-bootstrap.asciidoc, line 64 at r2 (raw file):

CSS, SCRIPT

STYLESHEET, JAVASCRIPT?


flow-server/src/main/java/com/vaadin/flow/server/InitialPageSettings.java, line 47 at r2 (raw file):

    }

    public enum WrapMode {

Missing javadoc here.


Comments from Reviewable

@caalador
Copy link
Contributor Author

caalador commented Jan 3, 2018

Review status: 6 of 8 files reviewed at latest revision, 2 unresolved discussions.


flow-documentation/application-structure/tutorial-bootstrap.asciidoc, line 64 at r2 (raw file):

Previously, ahie (Aleksi Hietanen) wrote…

CSS, SCRIPT

STYLESHEET, JAVASCRIPT?

Done.


flow-server/src/main/java/com/vaadin/flow/server/InitialPageSettings.java, line 47 at r2 (raw file):

Previously, ahie (Aleksi Hietanen) wrote…

Missing javadoc here.

Done.


Comments from Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


Comments from Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks broke.


flow-server/src/main/java/com/vaadin/flow/server/InitialPageSettings.java, line 47 at r3 (raw file):

Document this public enum by adding an explicit description.

Not sure why sonar is still complaining about this.


flow-server/src/main/java/com/vaadin/flow/shared/ui/Dependency.java, line 45 at r3 (raw file):

Document this public method by adding an explicit description.

+1, sorry missed this one.


Comments from Reviewable

@caalador
Copy link
Contributor Author

caalador commented Jan 3, 2018

Review status: 5 of 8 files reviewed at latest revision, 3 unresolved discussions.


flow-server/src/main/java/com/vaadin/flow/shared/ui/Dependency.java, line 45 at r3 (raw file):

Previously, ahie (Aleksi Hietanen) wrote…

Document this public method by adding an explicit description.

+1, sorry missed this one.

Done.


Comments from Reviewable

@ahie
Copy link
Contributor

ahie commented Jan 3, 2018

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 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. MAJOR BootstrapHandler.java#: This file has 760 lines, which is greater than 750 authorized. Split it into smaller files. rule
  2. INFO BootstrapHandler.java#L349: Return value of org.jsoup.nodes.Element.appendElement(String) ignored, is this OK in com.vaadin.flow.server.BootstrapHandler.getBootstrapPage(BootstrapHandler$BootstrapContext) rule

@ahie ahie merged commit 7686be0 into master Jan 3, 2018
@ahie ahie deleted the issues/3221_WrapMode branch January 3, 2018 12:50
@ahie ahie added this to the 1.0.0.alpha16 milestone Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants