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.

Sign up for GitHub

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

Jump to bottom

CCDM: connect client and server UIs from javascript #6210

Merged
merged 5 commits into from Aug 12, 2019
Merged

CCDM: connect client and server UIs from javascript #6210

merged 5 commits into from Aug 12, 2019

Conversation

manolo
Copy link
Member

@manolo manolo commented Aug 9, 2019 •
edited

Fixes part of #6133.

  • This Makes possible to append flow UI to browser without requiring an exported web component.
  • After flow-client configures the bootstrap, is it possible to attach UI to any slot in the browser.
  • This PR does not resolve client side navigation because server side is not implemented yet (Implement routing in JavaScriptBootstrapUI  #6221), instead it attaches a static view to the outlet from server
  • Fixes a couple of errors in the generated client javascript when importing the file as an ES6 module:
    • There was a JSNI block with non declared variable in a for statement
    • There was a weird function where a variable was not declared when the method was inlined by gwt compiler
  • Adding eslint rules to early detect whether the produced javascript has declaration errors

This change is Reviewable

@manolo manolo added the hilla Issues related to Hilla label Aug 9, 2019
@manolo manolo requested review from platosha and qtdzz August 9, 2019 11:39
@project-bot project-bot bot added this to Review in progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Aug 9, 2019
@manolo manolo force-pushed the mcm/ccdm/attach-server branch 2 times, most recently from d13b192 to 2362d55 Compare August 9, 2019 11:59
@manolo manolo self-assigned this Aug 9, 2019
@manolo manolo force-pushed the mcm/ccdm/attach-server branch 5 times, most recently from 5f12d72 to 2f5e5d0 Compare August 9, 2019 21:21
@manolo manolo changed the title CCDM: connect client and server side when navigating to a flow view CCDM: connect client and server UIs from javascript Aug 10, 2019
@manolo manolo force-pushed the mcm/ccdm/attach-server branch 2 times, most recently from c688a41 to bbc61c6 Compare August 12, 2019 08:47
platosha
platosha previously approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained (waiting on @qtdzz)

flow-server/src/test/java/com/vaadin/flow/dom/TestNodeVisitor.java, line 26 at r1 (raw file):

    private final boolean visitDescendants;

    public TestNodeVisitor(boolean visitDescendants) {

BTW Not an expert here, but these changes seem to be unnecessary.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Reviewer approved Aug 12, 2019
qtdzz
qtdzz previously approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@qtdzz qtdzz 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 7 of 7 files at r1.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

manolo
manolo commented Aug 12, 2019
View reviewed changes
Copy link
Member Author

@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: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

flow-server/src/test/java/com/vaadin/flow/dom/TestNodeVisitor.java, line 26 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

BTW Not an expert here, but these changes seem to be unnecessary.

These changes are needed, because I'm reusing an existing class for tests instead of writing my own.
Because there are access restrictions to properties, and my test class is in a different package, I needed to make public the api for this class.

Changes are in a class test, so there is no problem because we are not modifying the product API.

@manolo manolo dismissed stale reviews from qtdzz and platosha via 26c80b6 August 12, 2019 12:25
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Review in progress Aug 12, 2019
@manolo manolo closed this Aug 12, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Done - pending release Aug 12, 2019
@manolo manolo reopened this Aug 12, 2019
qtdzz
qtdzz approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

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

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Done - pending release to Reviewer approved Aug 12, 2019
@manolo manolo mentioned this pull request Aug 12, 2019
Closed
platosha
platosha approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@platosha platosha 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: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

platosha
platosha approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@platosha platosha left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@manolo manolo merged commit 25e39da into ccdm Aug 12, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Aug 12, 2019
@manolo manolo deleted the mcm/ccdm/attach-server branch August 12, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@platosha platosha platosha approved these changes

@qtdzz qtdzz qtdzz approved these changes

Assignees

@manolo manolo

Labels
hilla Issues related to Hilla
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@manolo @platosha @qtdzz