search

vaadin / vaadin-button-flow

Vaadin Flow Java API for vaadin/vaadin-button Web Component
https://vaadin.com/components/vaadin-button
Other
0 stars 8 forks source link

Adding a more convenient Constructor for VaadinIcon #112

Closed campbellbartlett closed 5 years ago

campbellbartlett commented 5 years ago

This change is Reviewable

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

vaadin-bot commented 5 years ago

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 Button.java#L49: Remove this unused "disableListener" private field. rule
denis-anisimov commented 5 years ago

Thanks for your contribution but components should not depend on each other except cases when it's a part of their generically functionality. Otherwise it may introduce circular dependencies and the existing functionality already allows to have what you want. It's not reasonable to have a constructor per every component which someone wants.

campbellbartlett commented 5 years ago

Ahh thats too bad, but you make some very strong points. Thanks for taking the time to review.

Legioth commented 5 years ago

I think it would be acceptable to make vaadin-button-flow depend on vaadin-icon-flow since buttons are clearly on a higher level of abstraction than icons which means that there isn't a big risk of circular dependencies.

On the other hand, I do agree that it would be unfortunate if the API would depend specifically on the icon set represented by VaadinIcon while not offering the same shorthand alternatives for other icon sets.

To deal with this, I would suggest a two-phased approach where vaadin-icon-flow is first updated to have an interface such as IconFactory (I'm not sure about that exact name) which is implemented by VaadinIcon but can also be implemented by any other icon set (as long as it depends on vaadin-icon-flow). The Button constructors could then accept an instance of IconFactory instead of being tied to a specific icon set implementation.

campbellbartlett commented 5 years ago

Were you thinking of an interface something along the lines of:

public interface HasIconFactory{ 
    Icon create();
}

this way, the current implementation of Vaadin Icon already adheres to the interface?

Legioth commented 5 years ago

Yes, that was what I had in mind.

campbellbartlett commented 5 years ago

I've raised a PR against Vaadin-Icons-Flow with the suggested changes. Once that PR is merged, I'll reopen this one and push a new commit that makes use of the new interface.

campbellbartlett commented 5 years ago

Hi, now that this PR is merged can we reopen this one?

Legioth commented 5 years ago

Since the actual solution will look slightly different now, I think it would be clearest if there would be a completely new PR instead of continuing on this one?