feat: implement collapse/flip/rotate as buttons AND feat: add commons-demo iconset#161
Draft
javier-godoy wants to merge 18 commits into
Draft
feat: implement collapse/flip/rotate as buttons AND feat: add commons-demo iconset#161javier-godoy wants to merge 18 commits into
javier-godoy wants to merge 18 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds an overlay button UI for the source code viewer, introduces new source collapse/position/orientation events and wiring in the demos, updates related component APIs, adds shared styling, and bumps project versions with a new clean profile. ChangesSource viewer buttons, events, and styling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Comment on lines
+368
to
+372
| private void setSourcePosition(SourcePosition sourcePosition, boolean fromClient) { | ||
| if (currentLayout != null) { | ||
| currentLayout.setSourcePosition(sourcePosition); | ||
| fireSourcePositionChangedEvent(sourcePosition, fromClient); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reapply collapsed splitter state after moving the source pane.
If sourceCollapsed is true, swapping the source between primary/secondary keeps the old splitter percentage. For example, a hidden secondary source at position 100 becomes a visible primary source after the swap. Call updateSplitterPosition() after changing the source position.
Proposed fix
private void setSourcePosition(SourcePosition sourcePosition, boolean fromClient) {
if (currentLayout != null) {
currentLayout.setSourcePosition(sourcePosition);
+ updateSplitterPosition();
fireSourcePositionChangedEvent(sourcePosition, fromClient);
}
}📝 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
| private void setSourcePosition(SourcePosition sourcePosition, boolean fromClient) { | |
| if (currentLayout != null) { | |
| currentLayout.setSourcePosition(sourcePosition); | |
| fireSourcePositionChangedEvent(sourcePosition, fromClient); | |
| } | |
| private void setSourcePosition(SourcePosition sourcePosition, boolean fromClient) { | |
| if (currentLayout != null) { | |
| currentLayout.setSourcePosition(sourcePosition); | |
| updateSplitterPosition(); | |
| fireSourcePositionChangedEvent(sourcePosition, fromClient); | |
| } |
🤖 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/TabbedDemo.java` around
lines 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.
Comment on lines
+407
to
+411
| private void setOrientation(Orientation orientation, boolean fromClient) { | ||
| if (currentLayout != null && orientation != getOrientation()) { | ||
| currentLayout.setOrientation(orientation); | ||
| currentLayout.setSourceCollapsed(false); | ||
| fireOrientationChangedEvent(orientation, fromClient); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset sourceCollapsed when orientation forces the source visible.
Line 410 expands the splitter, but the sourceCollapsed field remains true. The next updateSplitterPosition() will collapse it again, and listeners never receive a matching collapse-change event.
Proposed fix
private void setOrientation(Orientation orientation, boolean fromClient) {
if (currentLayout != null && orientation != getOrientation()) {
- currentLayout.setOrientation(orientation);
- currentLayout.setSourceCollapsed(false);
- fireOrientationChangedEvent(orientation, fromClient);
+ currentLayout.setOrientation(orientation);
+ if (sourceCollapsed) {
+ fireSourceCollapseChangedEvent(false, fromClient);
+ } else {
+ currentLayout.setSourceCollapsed(false);
+ }
+ fireOrientationChangedEvent(orientation, fromClient);
}
}📝 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
| private void setOrientation(Orientation orientation, boolean fromClient) { | |
| if (currentLayout != null && orientation != getOrientation()) { | |
| currentLayout.setOrientation(orientation); | |
| currentLayout.setSourceCollapsed(false); | |
| fireOrientationChangedEvent(orientation, fromClient); | |
| private void setOrientation(Orientation orientation, boolean fromClient) { | |
| if (currentLayout != null && orientation != getOrientation()) { | |
| currentLayout.setOrientation(orientation); | |
| if (sourceCollapsed) { | |
| fireSourceCollapseChangedEvent(false, fromClient); | |
| } else { | |
| currentLayout.setSourceCollapsed(false); | |
| } | |
| fireOrientationChangedEvent(orientation, fromClient); | |
| } | |
| } |
🤖 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/TabbedDemo.java` around
lines 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.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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