Skip to content

refactor: replace use of LumoUtility properties #16

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.

Sign up for GitHub

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

Merged
javier-godoy merged 1 commit into from
Oct 17, 2025
Merged

refactor: replace use of LumoUtility properties #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/main/java/com/flowingcode/vaadin/addons/cronexpressionfield/CronExpressionField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
import com.vaadin.flow.data.provider.DataProvider;
import com.vaadin.flow.function.SerializableConsumer;
import com.vaadin.flow.internal.JsonSerializer;
import com.vaadin.flow.theme.lumo.LumoUtility;

import elemental.json.JsonObject;
import org.springframework.scheduling.support.CronExpression;
import it.burning.cron.CronExpressionDescriptor;
Expand Down Expand Up @@ -196,10 +196,10 @@ private void configureLayout() {
cronExpressionLayoutCb.setWidth("40%");
cronDescriptionDiv.setWidthFull();
cronDescriptionDiv.setClassName("description-div");
addClassNames(LumoUtility.JustifyContent.CENTER, LumoUtility.AlignItems.CENTER, LumoUtility.Padding.NONE);
addClassName("fc-cron-expression-field");
Copy link

@coderabbitai coderabbitai bot Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

setMaxWidth("450px");

mainLayout.addClassNames(LumoUtility.Margin.NONE, LumoUtility.Padding.NONE);
mainLayout.addClassName("fc-cron-expression-field-main-layout");

topLayout.setWidthFull();
topLayout.setAlignItems(Alignment.END);
Expand Down Expand Up @@ -260,7 +260,7 @@ private void setAdvancedLayout() {

commonExpressionsCb.addValueChangeListener(v -> updateCronExpression(commonExpressionsCb.getValue()));
commonExpressionsCb.setWidthFull();
commonExpressionsCb.addClassName(LumoUtility.AlignSelf.CENTER);
commonExpressionsCb.addClassName("fc-common-expressions-combobox");
commonExpressionsCb.setLabel(i18n.getCommonExpressionsLabel());
commonExpressionsCb.setAllowCustomValue(false);
mainLayout.add(commonExpressionsCb);
Expand Down Expand Up @@ -305,7 +305,7 @@ private void setDailyLayout() {
getDailyExpression(timeUnitCb.getValue(), v.getValue());
}
});
timeAmountField.addClassName(LumoUtility.Margin.NONE);
timeAmountField.addClassName("fc-daily-layout-time-amount");
timeAmountField.setSuffixComponent(timeUnitCb);

DayOfWeekSelector daySelector = new DayOfWeekSelector();
Expand Down Expand Up @@ -364,7 +364,6 @@ private void setMonthlyLayout() {
}
HorizontalLayout layout = new HorizontalLayout(startsAtTp, dayOfMonthField, monthField);
layout.setWidthFull();
layout.setJustifyContentMode(JustifyContentMode.CENTER);
mainLayout.add(layout);
}

Expand Down
24 changes: 23 additions & 1 deletion src/main/resources/META-INF/frontend/styles/cron-expression-field-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,38 @@
margin: 0;
padding: 0;
}

.inline-combobox::part(input-field) {
background-color: transparent;
padding: 0;
margin: 0;
}

vaadin-grid::part(header) {
display: none;
}
Comment on lines 32 to 34
Copy link

@coderabbitai coderabbitai bot Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.java

Length 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.

Copy link
Member

@javier-godoy javier-godoy Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai please create an issue about it. The observation is valid but the issue is not introduced by this PR

Copy link

@coderabbitai coderabbitai bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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.

.description-div{

.description-div {
padding: 0;
margin: 0;
text-align: center;
}

.fc-cron-expression-field {
justify-content: center;
align-items: center;
padding: 0;
}

.fc-cron-expression-field-main-layout {
padding: 0;
margin: 0;
}

.fc-common-expressions-combobox {
align-self: center;
}

.fc-daily-layout-time-amount {
margin: 0;
}