feat: implement collapse/flip/rotate as buttons AND feat: add commons-demo iconset#161
Open
javier-godoy wants to merge 16 commits into
Open
feat: implement collapse/flip/rotate as buttons AND feat: add commons-demo iconset#161javier-godoy wants to merge 16 commits into
javier-godoy wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds source-viewer overlay controls with custom icons, event-driven collapse/position/orientation handling, updated SplitLayoutDemo and TabbedDemo integration, responsive styling, behavioral tests, inherited annotations, and Maven version/dependency updates. ChangesSource viewer overlay controls and demo integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
🧹 Nitpick comments (4)
base/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.css (2)
132-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment formatting nit flagged by stylelint.
Missing whitespace before
*/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.css` at line 132, The CSS comment in shared-styles.css is missing the required whitespace before the closing terminator, which is being flagged by stylelint. Update the comment formatting so the text inside the comment has a space before */ and matches the project’s linted comment style, using the nearby stylesheet comment near the icon rotation rule to locate it.Source: Linters/SAST tools
98-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclaration spacing flagged by stylelint.
declaration-empty-line-beforeexpects a blank line beforepaddingat line 100 per the project's stylelint config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.css` around lines 98 - 101, The stylelint rule on the shared-styles.css selector for vaadin-button.source-code-viewer-button vaadin-icon is flagging declaration spacing. Update that block so there is a blank line before the padding declaration, keeping the existing --vaadin-icon-size and padding declarations intact and matching the project’s declaration-empty-line-before convention.Source: Linters/SAST tools
base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts (1)
111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
home/githubicons.Neither icon is referenced by
CommonsDemoIcons(onlyROTATE,FLIP,HIDE_SOURCE,SHOW_SOURCEare defined). These appear to be leftovers from a copy-pasted template and add unused markup/license baggage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts` around lines 111 - 118, The <g id="home"> and <g id="github"> entries in CommonsDemoIconset are unused leftovers and should be removed from the sprite definition. Update commons-demo-iconset.ts to keep only the icons actually referenced by CommonsDemoIcons (ROTATE, FLIP, HIDE_SOURCE, SHOW_SOURCE) so the generated asset matches the exported API and avoids unnecessary markup.base/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.java (1)
67-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale Javadoc copied from another iconset.
{@codeBrands}doesn't describe this component; looks copy-pasted from a different icon set.✏️ Suggested fix
- /** - * Server side component for {`@code` Brands} - */ + /** + * Server side component for {`@code` CommonsDemoIcons}. + */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.java` around lines 67 - 69, The Javadoc on CommonsDemoIcons has stale copy-pasted text that references Brands instead of this icon set. Update the comment for the server-side component in CommonsDemoIcons so it describes the actual component/name being documented, using the existing class context rather than the copied {`@code` Brands} reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@base/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.java`:
- Around line 86-88: The MultiSourceCodeViewer.withButtons() path can
dereference codeViewer before it is initialized when all tabs are filtered out,
so add a guard for the empty-viewer case. Update the withButtons() method to
no-op or safely return this when codeViewer is absent, using the
MultiSourceCodeViewer constructor/empty-state setup and the codeViewer field as
the key locations to check.
In `@base/src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeViewer.java`:
- Around line 96-98: Update the Javadoc in SourceCodeViewer so it does not imply
the integrated button handling is purely client-side with no server involvement.
In the SourceCodeViewer documentation near the explanation of
source-code-viewer-buttons, clarify that while the web component dispatches DOM
events locally, TabbedDemo handles those events through Vaadin server-side
listeners. Keep the idempotent/added only once note, but remove or reword the
“no server roundtrip” statement to accurately reflect the server-side handling.
In `@base/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java`:
- Around line 368-372: In setSourcePosition(SourcePosition, boolean), the source
pane position is updated but the splitter state is not reapplied, so a collapsed
source can inherit the old percentage after swapping sides. After
currentLayout.setSourcePosition(...) and before/after
fireSourcePositionChangedEvent(...), call updateSplitterPosition() when
sourceCollapsed is true so the collapsed splitter position is restored
correctly. Use setSourcePosition and updateSplitterPosition in TabbedDemo to
locate the change.
- Around line 407-411: The `setOrientation(...)` flow in `TabbedDemo` expands
the splitter via `currentLayout.setSourceCollapsed(false)` but leaves the
`sourceCollapsed` state inconsistent, so the next `updateSplitterPosition()` can
re-collapse it and listeners miss the change. Update the orientation-handling
path in `setOrientation` (and any related `sourceCollapsed` state in
`currentLayout`) so forcing the source visible also resets the internal collapse
flag and emits the matching collapse-change event, keeping the layout state and
listeners in sync.
In
`@base/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.css`:
- Around line 132-151: The CSS in the shared-styles rules for
source-code-viewer-button uses an invalid transform function name. Update the
two source-code-viewer-rotate-button transform declarations in the
orientation-specific rules to use the standard CSS function name scaleX instead
of scalex, keeping the same rotate values and selectors.
In `@processor/pom.xml`:
- Line 8: Update the version alignment in processor/pom.xml so the processor
module and its related dependency property stay on the same release. In the POM,
bump the commons-demo.version property to match the new 5.4.0-SNAPSHOT
project/base version, and verify any dependency declarations that use that
property continue to point at the updated version. Focus on the version fields
in the POM rather than changing module structure.
---
Nitpick comments:
In `@base/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.java`:
- Around line 67-69: The Javadoc on CommonsDemoIcons has stale copy-pasted text
that references Brands instead of this icon set. Update the comment for the
server-side component in CommonsDemoIcons so it describes the actual
component/name being documented, using the existing class context rather than
the copied {`@code` Brands} reference.
In `@base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts`:
- Around line 111-118: The <g id="home"> and <g id="github"> entries in
CommonsDemoIconset are unused leftovers and should be removed from the sprite
definition. Update commons-demo-iconset.ts to keep only the icons actually
referenced by CommonsDemoIcons (ROTATE, FLIP, HIDE_SOURCE, SHOW_SOURCE) so the
generated asset matches the exported API and avoids unnecessary markup.
In
`@base/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.css`:
- Line 132: The CSS comment in shared-styles.css is missing the required
whitespace before the closing terminator, which is being flagged by stylelint.
Update the comment formatting so the text inside the comment has a space before
*/ and matches the project’s linted comment style, using the nearby stylesheet
comment near the icon rotation rule to locate it.
- Around line 98-101: The stylelint rule on the shared-styles.css selector for
vaadin-button.source-code-viewer-button vaadin-icon is flagging declaration
spacing. Update that block so there is a blank line before the padding
declaration, keeping the existing --vaadin-icon-size and padding declarations
intact and matching the project’s declaration-empty-line-before convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36cdc2dd-e712-4dee-8b65-d27d3e275d4a
📒 Files selected for processing (14)
base/pom.xmlbase/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeViewer.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/OrientationChangedEvent.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourceCollapseChangedEvent.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourcePositionChangedEvent.javabase/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.tsbase/src/main/resources/META-INF/resources/frontend/source-code-viewer-buttons.tsbase/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.csspom.xmlprocessor/pom.xml
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the intent of the unassigned
findElementcall.The call to
getDriver().findElement(By.id("content"));is unassigned and relies on throwing aNoSuchElementExceptionto fail the test if the element is missing. Wrapping this in an assertion (likeassertTrue) or an explicit wait clarifies the intent and can provide a better failure message.♻️ Proposed refactor
getCommandExecutor().waitForVaadin(); - getDriver().findElement(By.id("content")); + assertTrue("Content element should be present", hasElement("`#content`")); waitUntil(d -> hasElement("vaadin-button.source-code-viewer-hide-button"));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java` around lines 58 - 60, Clarify the content-element check in OverlayControlsIT by replacing the unassigned getDriver().findElement(By.id("content")) call with an explicit assertion or wait. Preserve the existing requirement that the test fails when the content element is absent, while providing a clear failure condition or message.base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts (1)
122-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the imported
Iconsetclass.The parameter
Iconsetin the.then()callback shadows theIconsetclass already imported at the top of the file (line 21). You can omit the parameter for cleaner and less confusing code.♻️ Proposed refactor
-customElements.whenDefined('vaadin-iconset').then(Iconset=>{ +customElements.whenDefined('vaadin-iconset').then(() => { Iconset.register('commons-demo', 24, template); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts` around lines 122 - 124, Update the customElements.whenDefined callback to omit its Iconset parameter and use the imported Iconset class directly when calling register, avoiding shadowing while preserving the existing commons-demo registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@base/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.ts`:
- Around line 122-124: Update the customElements.whenDefined callback to omit
its Iconset parameter and use the imported Iconset class directly when calling
register, avoiding shadowing while preserving the existing commons-demo
registration.
In
`@base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java`:
- Around line 58-60: Clarify the content-element check in OverlayControlsIT by
replacing the unassigned getDriver().findElement(By.id("content")) call with an
explicit assertion or wait. Preserve the existing requirement that the test
fails when the content element is absent, while providing a clear failure
condition or message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59292c4e-091f-4176-a3ef-e3f1259e8178
📒 Files selected for processing (23)
base/pom.xmlbase/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/DemoSources.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeViewer.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/OrientationChangedEvent.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourceCollapseChangedEvent.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourcePositionChangedEvent.javabase/src/main/resources/META-INF/resources/frontend/commons-demo-iconset.tsbase/src/main/resources/META-INF/resources/frontend/source-code-viewer-buttons.tsbase/src/main/resources/META-INF/resources/frontend/styles/commons-demo/shared-styles.cssbase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayCallables.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayView.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoUIUnitTest.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/ViewInitializerImpl.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/test/SplitLayoutDemoTest.javabase/src/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListenerpom.xmlprocessor/pom.xml
🚧 Files skipped from review as they are similar to previous changes (10)
- pom.xml
- base/src/main/java/com/flowingcode/vaadin/addons/demo/events/OrientationChangedEvent.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourcePositionChangedEvent.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/CommonsDemoIcons.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/events/SourceCollapseChangedEvent.java
- base/src/main/resources/META-INF/resources/frontend/source-code-viewer-buttons.ts
- base/src/main/java/com/flowingcode/vaadin/addons/demo/SourceCodeViewer.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java (1)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the field to match standard Java naming conventions.
The field name
$serverdoes not follow standard Java naming conventions. Consider renaming it toserverorserverProxyto align with the regular expression^[a-z][a-zA-Z0-9]*$and resolve the SonarCloud Code Analysis warning.♻️ Proposed fix
- private final OverlayCallables $server = createCallableProxy(OverlayCallables.class); + private final OverlayCallables serverProxy = createCallableProxy(OverlayCallables.class);(Note: Please also update all
$serverreferences in the test methods below.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java` at line 48, Rename the OverlayControlsIT field $server to server (or serverProxy) to comply with Java naming conventions, and update every reference to that field throughout the test methods while preserving the existing callable-proxy behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.java`:
- Line 48: Rename the OverlayControlsIT field $server to server (or serverProxy)
to comply with Java naming conventions, and update every reference to that field
throughout the test methods while preserving the existing callable-proxy
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50a17e92-e21d-4c5e-9dd5-52496a11d5ef
📒 Files selected for processing (15)
base/pom.xmlbase/src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/DemoSources.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SourcePosition.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.javabase/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayCallables.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayControlsIT.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayView.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoUIUnitTest.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoView.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/it/ViewInitializerImpl.javabase/src/test/java/com/flowingcode/vaadin/addons/demo/test/SplitLayoutDemoTest.javabase/src/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
🚧 Files skipped from review as they are similar to previous changes (11)
- base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayCallables.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/DemoSources.java
- base/src/test/resources/META-INF/services/com.vaadin.flow.server.VaadinServiceInitListener
- base/src/main/java/com/flowingcode/vaadin/addons/demo/MultiSourceCodeViewer.java
- base/src/test/java/com/flowingcode/vaadin/addons/demo/it/TabbedDemoUIUnitTest.java
- base/src/test/java/com/flowingcode/vaadin/addons/demo/it/OverlayView.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.java
- base/src/test/java/com/flowingcode/vaadin/addons/demo/it/ViewInitializerImpl.java
- base/src/test/java/com/flowingcode/vaadin/addons/demo/test/SplitLayoutDemoTest.java
- base/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java
- base/pom.xml
When the source is collapsed, swapping its side did not touch the splitter, so the collapsed pane inherited the previous side's percentage (e.g. showing the source full and hiding the demo instead of staying collapsed). Reapply the collapsed splitter position for the new side.
Changing the orientation forced the source visible via setSourceCollapsed(false) without updating sourceCollapsed or notifying listeners, leaving the layout and its state out of sync (e.g. a collapsed source was re-expanded on mobile). Reapply the current collapse state instead.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
commons-demoiconset and new demo icon factory.SourcePosition.DEFAULTwith improved inheritance/carry-over behavior.5.4.0-SNAPSHOTand refreshed test dependencies.