refactor: replace use of LumoUtility properties #16
Conversation
WalkthroughReplaces LumoUtility-based class names in CronExpressionField.java with custom CSS class names and removes a justify-content setting. Adds corresponding CSS rules in cron-expression-field-styles.css, including new selectors for layouts and controls, and minor selector additions/adjustments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.java (2)
103-103: Potential duplicate className assignment.Line 103 uses
this.setClassName("fc-cron-expression-field")while line 199 usesaddClassName("fc-cron-expression-field"). SincesetClassName()replaces all existing classes andaddClassName()appends, line 199 appears redundant. If the intent is to ensure the class is always present, consider removing line 199 to avoid confusion.Apply this diff to remove the duplicate:
cronDescriptionDiv.setWidthFull(); cronDescriptionDiv.setClassName("description-div"); - addClassName("fc-cron-expression-field"); setMaxWidth("450px");Also applies to: 199-199
199-308: Restore flex and alignment styles for main and monthly layouts
.fc-cron-expression-field-main-layoutin cron-expression-field-styles.css only sets padding/margin; you must add the equivalent of the removed API calls:.fc-cron-expression-field-main-layout { display: flex; /* adjust direction if needed: column or row */ justify-content: space-between; align-items: flex-end; }Also ensure the
HorizontalLayoutin setMonthlyLayout is centered—either via CSS or by restoringlayout.setJustifyContentMode(JustifyContentMode.CENTER).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.java(4 hunks)src/main/resources/META-INF/frontend/styles/cron-expression-field-styles.css(1 hunks)
🔇 Additional comments (12)
src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.java (6)
202-202: LGTM!The migration from LumoUtility classes to a custom CSS class is consistent with the PR objective.
263-263: LGTM!The custom CSS class replacement for the LumoUtility alignment is appropriate.
308-308: LGTM!The custom CSS class replacement for the LumoUtility margin is correct.
202-202: LGTM!Clean replacement of LumoUtility classes with a custom CSS class that follows the component's naming convention.
263-263: LGTM!Proper replacement of
LumoUtility.AlignSelf.CENTERwith a custom CSS class. Ensure the CSS file includes the necessary alignment styles.
308-308: LGTM!Proper replacement of
LumoUtility.Margin.NONEwith a custom CSS class. Verify the CSS file sets appropriate margin values.src/main/resources/META-INF/frontend/styles/cron-expression-field-styles.css (6)
26-30: LGTM!The styling correctly targets the input-field shadow part to maintain transparent background and reset spacing.
36-40: LGTM!Adding
text-align: centerto the description div improves the visual presentation.
42-46: LGTM!The new class provides appropriate centering and spacing for the cron expression field container.
48-51: LGTM!The reset of padding and margin for the main layout is appropriate.
53-55: LGTM!The
align-self: centerproperty correctly replaces the removed LumoUtility.AlignSelf.CENTER.
57-59: LGTM!The margin reset correctly replaces the removed LumoUtility.Margin.NONE.
| cronDescriptionDiv.setWidthFull(); | ||
| cronDescriptionDiv.setClassName("description-div"); | ||
| addClassNames(LumoUtility.JustifyContent.CENTER, LumoUtility.AlignItems.CENTER, LumoUtility.Padding.NONE); | ||
| addClassName("fc-cron-expression-field"); |
There was a problem hiding this comment.
Remove redundant class addition.
Line 103 already sets "fc-cron-expression-field" on this component via setClassName(). Adding it again here is unnecessary.
Apply this diff to remove the redundant line:
- addClassName("fc-cron-expression-field");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addClassName("fc-cron-expression-field"); |
🤖 Prompt for AI Agents
In
src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.java
around line 199, remove the redundant addClassName("fc-cron-expression-field");
call because the same class is already set earlier via setClassName() (line
~103); simply delete this line so the component class name is only applied once.
| vaadin-grid::part(header) { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the impact of globally hiding all grid headers.
This selector affects every vaadin-grid component in the application, not just the one in CronExpressionField. If other grids need their headers, this will break them.
Run the following script to check if vaadin-grid is used elsewhere:
🏁 Script executed:
#!/bin/bash
# Description: Find all usages of vaadin-grid to assess impact of global header hiding
# Search for Grid component instantiation
rg -nP --type=java 'new\s+Grid<' -C3
# Search for vaadin-grid in HTML/templates
fd -e html -e css | xargs rg -n 'vaadin-grid'Length of output: 1115
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.javaLength of output: 7400
Scope grid header hiding to CronExpressionField only
The @CssImport is loaded globally by default, so vaadin-grid::part(header) currently hides headers in every grid. Prefix the selector with the component’s host class (e.g. .fc-cron-expression-field vaadin-grid::part(header) { display: none; }) or adjust the import to limit its scope.
🤖 Prompt for AI Agents
In src/main/resources/META-INF/frontend/styles/cron-expression-field-styles.css
around lines 32 to 34, the selector vaadin-grid::part(header) is global and
hides headers in every Vaadin grid; scope it to the CronExpressionField by
prefixing the selector with the component host class (for example use
.fc-cron-expression-field vaadin-grid::part(header) { display: none; }) or
alternatively change the @CssImport to be applied only to the
CronExpressionField component so the rule only affects that component.
There was a problem hiding this comment.
@coderabbitai please create an issue about it. The observation is valid but the issue is not introduced by this PR
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create GitHub issue about global vaadin-grid header CSS selector
gh issue create \
--title "Scope vaadin-grid header hiding to CronExpressionField only" \
--body "The CSS rule \`vaadin-grid::part(header) { display: none; }\` in \`cron-expression-field-styles.css\` is currently global and affects every vaadin-grid component in applications using CronExpressionField.
This selector should be scoped to only affect grids within CronExpressionField to prevent unintended side effects on other grids in the application.
**Context:**
- Issue identified during review of PR #16
- Comment: https://github.com/FlowingCode/CronExpressionField/pull/16#discussion_r2417830675
- Requested by: @javier-godoy
This is a pre-existing issue not introduced by PR #16." \
--assignee javier-godoy
Length of output: 60
@javier-godoy I've created issue #19 to track the global CSS selector scoping problem. Thanks for clarifying that this is a pre-existing issue separate from the current refactoring work.
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #10
Summary by CodeRabbit