-
Notifications
You must be signed in to change notification settings - Fork 72
feat: add Aura variants #8145
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
base: main
Are you sure you want to change the base?
feat: add Aura variants #8145
Conversation
...aadin-dashboard-flow/src/main/java/com/vaadin/flow/component/dashboard/DashboardVariant.java
Outdated
Show resolved
Hide resolved
...ent/vaadin-grid-pro-flow/src/main/java/com/vaadin/flow/component/gridpro/GridProVariant.java
Outdated
Show resolved
Hide resolved
...ent/vaadin-menu-bar-flow/src/main/java/com/vaadin/flow/component/menubar/MenuBarVariant.java
Show resolved
Hide resolved
LUMO_DROPDOWN_INDICATORS("dropdown-indicators"); | ||
LUMO_DROPDOWN_INDICATORS("dropdown-indicators"), | ||
TERTIARY("tertiary"), | ||
ICON("icon"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a base style variant, keep the Lumo version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is there in the base button styles like with the Lumo version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant the specific menu bar button in https://github.com/vaadin/web-components/blob/main/packages/menu-bar/src/styles/vaadin-menu-bar-button-base-styles.js#L31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, thanks for the heads up. I don't think I’ve noticed that. I’d say we should remove the icon variant from that selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made at least a note to myself to update that selector during beta.
...dered-layout-flow/src/main/java/com/vaadin/flow/component/orderedlayout/ScrollerVariant.java
Outdated
Show resolved
Hide resolved
/** | ||
* @deprecated Use {@link #VERTICAL} instead. | ||
*/ | ||
@Deprecated(since = "25.0", forRemoval = true) | ||
LUMO_VERTICAL("vertical"), | ||
/** | ||
* @deprecated Use {@link #HELPER_ABOVE_FIELD} instead. | ||
*/ | ||
@Deprecated(since = "25.0", forRemoval = true) | ||
LUMO_HELPER_ABOVE_FIELD("helper-above-field"), | ||
VERTICAL("vertical"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be the same as Checkbox Group, that the base variant is horizontal
, but Lumo has the opposite (default is horizontal and the variant is vertical).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it along with the uses in the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a bit weird IMO, as HORIZONTAL
doesn't do anything for Lumo, same as VERTICAL
doesn't do anything in base / Aura. Would look cleaner if we'd just have both HORIZONTAL
and VERTICAL
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that sounds okay to me, even if VERTICAL is basically a no-op in base styles and Aura (and same for HORIZONTAL in Lumo).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starts to sound like an orientation
property more than a theme variant, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR to include both horizontal and vertical for radio button group and checkbox group variants.
c1ae768
to
9727063
Compare
f11629d
to
bb5fdb9
Compare
The failing test on CI ( |
*/ | ||
@Deprecated(since = "25.0", forRemoval = true) | ||
LUMO_NO_PADDING("no-padding"), | ||
NO_BORDER("no-border"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-border
is basically a no-op in Lumo since it doesn't have a border.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the same. However maybe it's better to keep this as is for simplicity. Since Lumo doesn't have a border, it's also unlikely that you would try to use it there, or expect it to do anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not a problem IMO, just wanted to point it out to confirm the agreement in such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, would make sense to change the default in Lumo, but probably not worth the breaking change for users.
*/ | ||
public enum SplitLayoutVariant implements ThemeVariant { | ||
LUMO_SMALL("small"), LUMO_MINIMAL("minimal"); | ||
LUMO_SMALL("small"), LUMO_MINIMAL("minimal"), SMALL("small"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing Lumo deprecation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the missing annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR tags #8126 as fixed, however it's still missing the Aura specific variants. I was assuming those would come in a follow-up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could update it as "part of". I tagged it so because it seemed like the scope of the issue was reduced to base variants (possibly temporarily) in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add something for Aura at some point, so since we have no other ticket for that I think we should keep this one.
I'm also still a bit confused since we discussed to use class names instead of theme variants for Aura at some point. But that makes it rather inconsistent with the base variants being a theme variant, and Aura variants being a class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR tag as "part of".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sissbruecker: the only classnames in Aura at the moment are .aura-surface
and .aura-surface-solid
, which are general purpose for any element, not just our components. So it doesn't make sense to have those use the theme attribute (because of React mainly).
LUMO_NO_ROW_BORDERS("no-row-borders"), | ||
LUMO_COLUMN_BORDERS("column-borders"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Aura also supports these, but base styles do not. Should there be separate prefixed variants for Aura, or should we have this as a base variant?
Maybe more of a question for @jouni
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base styles leaves this as a responsibility of a theme, to decide what the default border configuration is (whether to show column and row borders or not). And then it’s naturally also a theme’s responsibility to offer any variants that allow to deviate from those defaults easily per component.
I don't want to force all themes to implement grid borders the same way by forcing them to implement those base variants.
So it seems we’ll have these ones for both Lumo and Aura.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A similar case could be made about row stripes, but it felt a bit more natural to force that default than the borders. Well, maybe the same case can be made about almost all variants. I suppose I’m just struggling to define a clear guideline how we decide what goes to base styles and what is left for themes. Suggestions welcome! Ping @rolfsmeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it is arguably a bit late to make major changes, but the more I think about it, the more it feels like base styles should have as few variants as possible, like things that should maybe not be theme variants at all (like orientation) and a few really obvious ones.
640ed49
to
3440633
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some ambiguity regarding what is a base style variant, but that’s something I think we can still fine tune later.
Decision: No base variants should be exposed, all variants should have theme prefixes (those that are present in both themes should be duplicated) |
3440633
to
32c7a49
Compare
|
Is the reasoning for this decision public? |
No, it's a super secret strategic business decision 😁 Jokes aside, it's primarily that variants implemented in base styles are available in all themes, so duplicating them would be kind of pointless. We initially considered having unprefixed variants of base variants in the |
Understandable reasoning! I personally was hoping for an universal "ButtonVariant.PRIMARY" enum that just works (1) regardless of which theme I use (lumo, aura or something else) this feels more naturally instead of remembering (always adding AURA_) in front of all values. (AI is probably getting this wrong as well.. half joking) Yes, that leaves us with the problem that either theme specific variants have to be prefixed or they just don't work on all themes. I'm not sure how many theme-specific variants there are... So I would just say: no prefix and just add java doc information if it does not apply to all themes if you don't want to create theme specific enum classes (1) I know it already works like this with LUMO_PRIMARY and AURA_PRIMARY cause both map to primary on the client. Edit: RIP my hope for deprecated LUMO-Variants 🤭 |
Description
This PR
HasThemeVariant
interface toUpload
SideNav
MessageInput
Closes #8126
Type of change
Checklist