Uh oh!
There was an error while loading. Please reload this page.
Add Vaadin 25 support while maintaining Vaadin 24 compatibility - #28
Merged
Conversation
Extract JavaScript DOM traversal into LoginOverlayUtils helper methods. Update LoginLayout and ReplaceableLoginOverlay to use these methods, and add removeForgotPassword() extracted from replaceForgotPassword(). Co-Authored-By: Javier Godoy <11554739+javier-godoy@users.noreply.github.com>
Add Vaadin 25 fallback logic to all LoginOverlayUtils helper methods.
When getElementById('vaadinLoginOverlayWrapper') returns null (Vaadin 25),
fall back to querying the vaadin-login-overlay shadow DOM for the wrapper.
Close#23
Co-Authored-By: Javier Godoy <11554739+javier-godoy@users.noreply.github.com>Use demo source condition attribute to select between Vaadin 24 & Vaadin 25 styles.
WalkthroughAdds Vaadin 25.0.6 compatibility changes: new DOM-manipulation utilities, ReplaceableLoginOverlay API additions, RPC-based server-version support in integration tests, version-gated demo/styles, new integration test views, and Maven/test dependency updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java (1)
76-83: Consider more specific validation feedback for demo clarity.The current validation conflates two failure cases under one message:
- Password is null/blank → "Passwords do not match."
- Passwords actually don't match → "Passwords do not match."
For a demo showcasing form validation patterns, slightly more specific messages could be helpful, though the current implementation is functional.
♻️ Optional: More specific validation messages
private void onAccept() { String passwordValue = this.password.getValue(); String confirmPasswordValue = this.confirmPassword.getValue(); - if (passwordValue != null && !passwordValue.isBlank() && passwordValue.equals(confirmPasswordValue)) {+ if (passwordValue == null || passwordValue.isBlank()) {+ Notification.show("Password cannot be empty.");+ } else if (!passwordValue.equals(confirmPasswordValue)) {+ Notification.show("Passwords do not match.");+ } else { Notification.show("Password changed."); - } else {- Notification.show("Passwords do not match."); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java` around lines 76 - 83, The onAccept method currently treats a blank/null password and a mismatched confirmation the same; update onAccept to first check that password.getValue() is non-null and not blank and show a specific message like "Password cannot be blank" (or set a validation error) if it fails, then separately check password.equals(confirmPasswordValue) and show "Passwords do not match" only in that case; locate the onAccept method and the variables password, confirmPassword, passwordValue and confirmPasswordValue to implement the two-step validation and corresponding Notification messages.src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java (1)
53-53: Tighten the RPC proxy field contract.Subclasses rely on this proxy, so leaving it package-private and mutable is looser than necessary. Marking it
protected finalmakes the intended extension point explicit and prevents accidental reassignment.♻️ Proposed tweak
- ServerVersionCallables $server = createCallableProxy(ServerVersionCallables.class);+ protected final ServerVersionCallables $server = createCallableProxy(ServerVersionCallables.class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java` at line 53, The field holding the RPC proxy (ServerVersionCallables $server) is currently package-private and mutable; change its declaration to protected final to tighten the contract so subclasses can use but not reassign it. Locate the declaration of ServerVersionCallables $server in AbstractViewTest (the line with createCallableProxy(ServerVersionCallables.class)) and update its modifier to protected final, ensuring any initialization remains compatible with the constructor or field initializer that calls createCallableProxy.src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java (1)
58-77: You can merge brand clear+append into a singleexecuteJscall.This would avoid two client round-trips and remove timing coupling between separate async snippets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java` around lines 58 - 77, The code issues two separate getElement().executeJs calls to first clear and then append to the brand element causing extra round-trips and timing coupling; merge them into a single executeJs invocation in ReplaceableLoginOverlay that uses LoginOverlayUtils.getOverlayWrapperScript to both clear (replaceChildren()) and append ($0) the passed withElement in one script, keeping the same parameter (withElement) and call site so the operation is atomic and reduces client-server round-trips.src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java (1)
39-54: Consider extracting shared overlay-wrapper lookup to one internal template.The Vaadin 24/25 wrapper resolution block is duplicated in two methods. Centralizing it would reduce drift risk when compatibility logic changes again.
Also applies to: 88-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java` around lines 39 - 54, Extract the duplicated Vaadin 24/25 overlay-wrapper lookup into a single internal template/constant and reuse it from getOverlayWrapperScript and the other method that contains the same block (the overlay-close/cleanup script method), so the JS snippet is defined once and then injected (e.g., via String.format or text block concatenation) into both getOverlayWrapperScript and the sibling method; update both methods to reference the new shared symbol (e.g., OVERLAY_WRAPPER_LOOKUP or buildOverlayWrapperLookup()) and remove the duplicated JS block from each method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayout.java`:
- Around line 54-65: Guard the usage of the content field in the onAttach logic
by checking content for null before executing the form replacement JS and DOM
manipulations: wrap the block that calls
this.getElement().executeJs(LoginOverlayUtils.getLoginFormWrapperScript(...)),
this.getElement().appendChild(content.getElement()),
content.getElement().setAttribute("slot","form"), and
content.getElement().executeJs(...) with an early-return or conditional (e.g.,
if (content == null) return; or if (content != null) { ... }); ensure you
reference the same symbols (content, this.getElement(),
LoginOverlayUtils.getLoginFormWrapperScript, formWrapper) so the DOM/script
operations only run when content is present.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.java`:
- Around line 20-29: The ServerVersionCallables interface is currently in the
integration-test (.it) package and is referenced by TestExtendedLoginOverlayView
and TestLoginLayout, which are included in the demo JAR; move the contract to a
neutral test-support package so the demo classifier can load the tests without
missing references. Create a new package (e.g.,
com.flowingcode.vaadin.addons.extendedlogin.testsupport) and relocate the
ServerVersionCallables interface there (keep the same interface name and
getMajorVersion() signature), then update imports in
TestExtendedLoginOverlayView and TestLoginLayout to point to the new package;
alternatively, if you prefer not to move files, ensure the interface is
explicitly included in the demo JAR by adjusting the build configuration to not
exclude it.
In `@src/test/resources/META-INF/frontend/styles/extended-login-styles-v25.css`:
- Line 1: The comment-whitespace-inside stylelint violations are caused by
comments that lack required spaces; update the comment tokens in this file by
inserting a space after the opening /* and before the closing */ — e.g., change
the leading comment token that is currently "/*-" to include a space ("/* -")
and change "/*Demo styles*/" to "/* Demo styles */" so they comply with the
comment-whitespace-inside rule; locate these comments in the CSS and adjust
their spacing accordingly.
---
Nitpick comments:
In
`@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.java`:
- Around line 39-54: Extract the duplicated Vaadin 24/25 overlay-wrapper lookup
into a single internal template/constant and reuse it from
getOverlayWrapperScript and the other method that contains the same block (the
overlay-close/cleanup script method), so the JS snippet is defined once and then
injected (e.g., via String.format or text block concatenation) into both
getOverlayWrapperScript and the sibling method; update both methods to reference
the new shared symbol (e.g., OVERLAY_WRAPPER_LOOKUP or
buildOverlayWrapperLookup()) and remove the duplicated JS block from each
method.
In
`@src/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.java`:
- Around line 58-77: The code issues two separate getElement().executeJs calls
to first clear and then append to the brand element causing extra round-trips
and timing coupling; merge them into a single executeJs invocation in
ReplaceableLoginOverlay that uses LoginOverlayUtils.getOverlayWrapperScript to
both clear (replaceChildren()) and append ($0) the passed withElement in one
script, keeping the same parameter (withElement) and call site so the operation
is atomic and reduces client-server round-trips.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java`:
- Line 53: The field holding the RPC proxy (ServerVersionCallables $server) is
currently package-private and mutable; change its declaration to protected final
to tighten the contract so subclasses can use but not reassign it. Locate the
declaration of ServerVersionCallables $server in AbstractViewTest (the line with
createCallableProxy(ServerVersionCallables.class)) and update its modifier to
protected final, ensuring any initialization remains compatible with the
constructor or field initializer that calls createCallableProxy.
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.java`:
- Around line 76-83: The onAccept method currently treats a blank/null password
and a mismatched confirmation the same; update onAccept to first check that
password.getValue() is non-null and not blank and show a specific message like
"Password cannot be blank" (or set a validation error) if it fails, then
separately check password.equals(confirmPasswordValue) and show "Passwords do
not match" only in that case; locate the onAccept method and the variables
password, confirmPassword, passwordValue and confirmPasswordValue to implement
the two-step validation and corresponding Notification messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe931338-0769-4cc1-960e-95dcf11c9083
📒 Files selected for processing (15)
pom.xmlsrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayout.javasrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/LoginOverlayUtils.javasrc/main/java/com/flowingcode/vaadin/addons/extendedlogin/ReplaceableLoginOverlay.javasrc/test/java/com/flowingcode/vaadin/addons/AppShellConfiguratorImpl.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/ExtendedLoginOverlayDemo.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/LoginLayoutDemo.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestExtendedLoginOverlayView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayout.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayoutView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ExtendedLoginOverlayIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/LoginLayoutIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.javasrc/test/resources/META-INF/frontend/styles/extended-login-styles-v25.css
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Add testbench-rpc dependency to be able to retrieve Vaadin version.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java (1)
53-54: Consider renaming$serverto follow Java naming conventions (optional).SonarCloud flags the
$prefix as non-standard. While$serveris a recognizable convention in some TestBench RPC contexts to denote server proxies, renaming toserverorserverProxywould satisfy the linter and remain clear.🔧 Suggested rename
- ServerVersionCallables $server = createCallableProxy(ServerVersionCallables.class);+ ServerVersionCallables server = createCallableProxy(ServerVersionCallables.class);Note: If you apply this rename, update all usages in subclasses (e.g.,
$server.getMajorVersion()→server.getMajorVersion()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java` around lines 53 - 54, The variable name $server uses a non-standard `$` prefix; rename the field/variable to a conventional identifier (e.g., server or serverProxy) where it is declared (the ServerVersionCallables $server assignment) and update all usages throughout the test class and subclasses (for example replace $server.getMajorVersion() with server.getMajorVersion()) to keep references consistent and satisfy the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.java`:
- Around line 53-54: The variable name $server uses a non-standard `$` prefix;
rename the field/variable to a conventional identifier (e.g., server or
serverProxy) where it is declared (the ServerVersionCallables $server
assignment) and update all usages throughout the test class and subclasses (for
example replace $server.getMajorVersion() with server.getMajorVersion()) to keep
references consistent and satisfy the linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9340e2b9-4ea5-404b-a94b-f728bf492e53
📒 Files selected for processing (10)
pom.xmlsrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestExtendedLoginOverlayView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/TestLoginLayout.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/AbstractViewTest.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ExtendedLoginOverlayIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/LoginLayoutIT.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/TestExtendedLoginOverlayIntegrationView.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/TestLoginLayoutIntegration.javasrc/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/TestLoginLayoutIntegrationView.java
🚧 Files skipped from review as they are similar to previous changes (4)
- pom.xml
- src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ServerVersionCallables.java
- src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/TestLoginLayoutIntegration.java
- src/test/java/com/flowingcode/vaadin/addons/extendedlogin/it/ExtendedLoginOverlayIT.java
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR adds Vaadin 25 compatibility while maintaining support for Vaadin 24, along with several refactorings and demo improvements. (Rebased from #27)
Changes:
Vaadin 24 & 25 compatibility
Code improvements
Demo improvements
Summary by CodeRabbit
New Features
Improvements
Chores