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

Updates all components to stable version #4578

Merged
merged 3 commits into from
Sep 24, 2018

Conversation

ZheSun88
Copy link
Contributor

@ZheSun88 ZheSun88 commented Sep 3, 2018

This is part of the component update. vaadin/flow-component-base#77


This change is Reviewable

@ZheSun88 ZheSun88 modified the milestones: 1.0.6, 1.1.0.beta3 Sep 3, 2018
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Reviewed 99 of 99 files at r1.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/button/GeneratedVaadinButton.java, line 202 at r1 (raw file):

    }

    protected void updateStyles() {

Is it ok that we have some methods disappearing from our Java API?


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/combobox/GeneratedVaadinComboBoxDropdown.java, line 252 at r1 (raw file):

     * @return a {@link Registration} for removing the event listener
     */
    protected Registration addOpenedChangeListener(

This looks even more dangerous then updateStyles disappearance.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Sep 4, 2018
Copy link
Contributor

@pleku pleku 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


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/button/GeneratedVaadinButton.java, line 202 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Is it ok that we have some methods disappearing from our Java API?

OK since it is protected but according to semver this is a breaking change and thus warrants a new major version of the component (protected Java API is public user facing API).

Since it is the generated class, we could have claimed that using these methods is not


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/combobox/GeneratedVaadinComboBoxDropdown.java, line 252 at r1 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

This looks even more dangerous then updateStyles disappearance.

Again - breaking change.
Should ask the components team on why (?) and how they make this change between beta and final ?

Copy link
Contributor

@pleku pleku 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


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/button/GeneratedVaadinButton.java, line 202 at r1 (raw file):

Previously, pleku (Pekka Hyvönen) wrote…

OK since it is protected but according to semver this is a breaking change and thus warrants a new major version of the component (protected Java API is public user facing API).

Since it is the generated class, we could have claimed that using these methods is not

This a polymer method that we are not using - and neither should be anyone else. I'd say it should have been excluded from the generator anyway, but now it has been here until this point, which is unfortunate.

The usage of generated classes is still perceived to be "at your own risk" and the generator template should be updated to refrain that in the class level javadocs.
I'm going to let this change slide in this minor.


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/combobox/GeneratedVaadinComboBoxDropdown.java, line 252 at r1 (raw file):

Previously, pleku (Pekka Hyvönen) wrote…

Again - breaking change.
Should ask the components team on why (?) and how they make this change between beta and final ?

This is actually cb-dropdown, and not used anywhere.

Copy link
Contributor

@pleku pleku 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: 3 unresolved discussions, 0 of 1 LGTMs obtained

a discussion (no related file):
Blocking this for now, pending inclusion decision for certain 1.1 version


@pekam pekam removed this from the 1.1.0.beta3 milestone Sep 5, 2018
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore 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, 0 of 1 LGTMs obtained


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/button/GeneratedVaadinButton.java, line 202 at r1 (raw file):

Previously, pleku (Pekka Hyvönen) wrote…

This a polymer method that we are not using - and neither should be anyone else. I'd say it should have been excluded from the generator anyway, but now it has been here until this point, which is unfortunate.

The usage of generated classes is still perceived to be "at your own risk" and the generator template should be updated to refrain that in the class level javadocs.
I'm going to let this change slide in this minor.

Ok.


flow-components-parent/flow-generated-components/src/main/java/com/vaadin/flow/component/combobox/GeneratedVaadinComboBoxDropdown.java, line 252 at r1 (raw file):

Previously, pleku (Pekka Hyvönen) wrote…

This is actually cb-dropdown, and not used anywhere.

Ok.

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore 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 19 of 19 files at r2.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained, and 1 stale

Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained, and 1 stale

Copy link
Contributor

@pleku pleku 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, 2 of 1 LGTMs obtained

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.

4 participants