-
Notifications
You must be signed in to change notification settings - Fork 1
feat: deprecate get/applyThemeAttribute in favor of get/setColorScheme #138
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
Conversation
WalkthroughVersion bump from 5.1.1-SNAPSHOT to 5.2.0-SNAPSHOT in pom.xml. Introduction of a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/main/java/com/flowingcode/vaadin/addons/demo/ColorScheme.java`:
- Around line 22-36: Remove the accidental Vaadin license block that was
inserted between imports: delete the entire comment block located between the
import lines for lombok.Getter and lombok.RequiredArgsConstructor so imports are
contiguous; ensure the file still imports lombok.Getter and
lombok.RequiredArgsConstructor and that the ColorScheme class and its
annotations remain unaffected.
In `@src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java`:
- Around line 346-348: getOrientation() can NPE because it calls
currentLayout.getOrientation() without checking currentLayout; update the
getOrientation method to mirror the defensive checks used in
toggleSourcePosition/setOrientation by returning a safe default (e.g., null or a
default Orientation) when currentLayout is null or otherwise handling the null
case, so reference currentLayout and getOrientation in your change and align
behavior with toggleSourcePosition/setOrientation.
- Around line 400-407: The applyThemeAttribute method uses
element.getComponent().get() unsafely and silently ignores unknown themes;
update applyThemeAttribute to check element.getComponent().isPresent() (or use
orElseThrow with a clear message) before obtaining the Component, and ensure
unknown theme strings fall back to a defined behavior (e.g., call
setColorScheme(c, ColorScheme.LIGHT) or log/warn and leave unchanged) rather
than doing nothing; reference the Element#getComponent() Optional handling and
the setColorScheme(Component, ColorScheme) and ColorScheme enum to locate the
changes.
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (1)
315-318: Potential visibility state inconsistency.
setSourceVisible(true)always makescodePositionCBvisible, butupdateFooterButtonsVisibility()sets its visibility based on whethercurrentLayout != null. This could lead to inconsistent states wherecodePositionCBis visible even when there's no source code layout.Consider aligning the visibility logic or delegating to
updateFooterButtonsVisibility().♻️ Proposed fix
public void setSourceVisible(boolean visible) { codeCB.setValue(visible); - codePositionCB.setVisible(visible); + codePositionCB.setVisible(visible && currentLayout != null); }
|



In Vaadin 25, the framework officially uses the term ColorScheme (light/dark). Previously, our codebase used "themeAttribute," which has become ambiguous now that we’re considering to implement dynamic theme switching. I’ve updated the naming to align with Vaadin’s nomenclature and to make the distinction between a Theme (the whole look) and a ColorScheme (the specific color mode) clearer.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.