feat: add API to set spacing in HL/VL with custom values #7129
Conversation
This changes adds: - `setSpacing(String)` - method to set spacing passing a CSS lenght value (e.g. `10px`, `1em`) - `setSpacing(float, Unit)` - method to set spacing passing a float value and a `Unit` - `getSpacing()` - method to return the value set to the `gap` CSS property of the layout It also changes `isSpacing()` to return whether the spacing has been set by the theme API or by setting the `gap` property.
...rdered-layout-flow/src/main/java/com/vaadin/flow/component/orderedlayout/ThemableLayout.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @return the spacing between the components | ||
| */ | ||
| default String getSpacing() { |
There was a problem hiding this comment.
It looks like this wouldn't return a value when:
- Using
setSpacing(boolean) - Using
setSpacing(SpacingEnum)once that is added
Which could be somewhat confusing? I wonder if we need this, or if we should clarify this in the JavaDoc at least.
There was a problem hiding this comment.
Indeed the setSpacing(boolean) wouldn't return a value, but setSpacing(SpacingEnum) depends on the implementation. For instance, each enum entrance could hold the name of the CSS variable it maps to, then the setSpacint(SpacingEnum) would call setSpacing(enum.getValue()). For the boolean API, the isSpacing() would be the preferable way to check whether spacing is set or not, then we could mention it on the JavaDoc.
| @@ -93,16 +94,58 @@ default boolean isPadding() { | |||
| * it if {@code false} | |||
| */ | |||
| default void setSpacing(boolean spacing) { | |||
| if (!spacing) { | |||
| getElement().getStyle().remove("gap"); | |||
There was a problem hiding this comment.
Could add a test case for this.
| public void checkSpacingStringSetter() { | ||
| layout.setSpacing("20px"); | ||
| assertTrue("Expected spacing to be applied after setting it", | ||
| layout.isSpacing()); |
There was a problem hiding this comment.
isSpacing checks for both, the theme variant or the gap. It looks like the new overloads should also always set the theme variant, so maybe it would be good to cover that explicitly.
There was a problem hiding this comment.
Actually it's not needed to set the theme in the new overloads. It was a leftover from another approach that would be require a few changes in the web component. Removed.
|
Description
This change adds:
setSpacing(String)- method to set spacing passing a CSS length value (e.g.10px,1em)setSpacing(float, Unit)- method to set spacing passing a float value and aUnitgetSpacing()- method to return the value set to thegapCSS property of the layoutIt also changes
isSpacing()to return whether the spacing has been set by the theme API or by setting thegapproperty.Part of #6999 - the enum API is not part of this PR and should be added later
Type of change