feat!: allow changing the icon #4594
Conversation
|
SonarCloud Quality Gate failed.
|
|
SonarCloud Quality Gate failed.
|
There was a problem hiding this comment.
Allowing the developer to change the icon sounds reasonable to me. I'm slightly concerned that there is a chance someone would request a getter in the future as there is currently no way to know what icon the component displays. I wonder what such a getter could look like given the current setter API.
public String getCollection();
public String getIcon();
There was a problem hiding this comment.
We discussed this PR internally and arrived at the following outcome:
-
setIcon(String icon)should re-use the previous collection. It can be unexpected otherwise thatsetIcon(String icon)resets the collection toICON_COLLECTION_NAMEfor Icons initialized with a custom collection. -
setIcon(VaadinIcon icon)is also required. UsingVaadinIconis pretty common among Flow developers. - The new API should be covered with unit tests.
|
Closing the PR as stale as the last review comments have not been addressed in over a month. Please don't hesitate to open a new PR if you want to continue with this contribution. Thanks! |
|
Updated based on review comments |
...n-icons-flow-parent/vaadin-icons-flow/src/main/java/com/vaadin/flow/component/icon/Icon.java
Show resolved
Hide resolved
...n-icons-flow-parent/vaadin-icons-flow/src/main/java/com/vaadin/flow/component/icon/Icon.java
Outdated
Show resolved
Hide resolved
…aadin/flow/component/icon/Icon.java Co-authored-by: Sergey Vinogradov <[email protected]>
| } | ||
|
|
||
| @Test | ||
| public void canSetNewVaadinIcon() { |
There was a problem hiding this comment.
nit: Since getCollection is a public method, it would be great to have a test for it too
|
Co-authored-by: Artur <[email protected]>
|
This ticket/PR has been released with Vaadin 24.5.0.alpha1 and is also targeting the upcoming stable 24.5.0 version. |
Description
Icon is missing a setter for the icon
Warning
Note, it's a minor behavior altering change because the default constructor is changed to not set the default icon.
Type of change
Checklist
Additional for
Featuretype of change