Apply application's theme to MarkdownEditor component #15
Conversation
WalkthroughThis pull request updates the project version in the Changes
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownEditorDemo.java (1)
52-65: Good implementation of application-wide theme handlingThe code now properly manages theme at the application level rather than just component level, which aligns with the PR objective. The implementation correctly handles all three cases (dark, light, and automatic) by manipulating the UI theme list.
One minor suggestion to consider - explicitly handling the default case with a notification or log entry might be helpful for debugging:
default: + System.out.println("Unrecognized theme option: " + ev.getValue()); break;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/markdown/BaseMarkdownComponent.java(3 hunks)src/main/resources/META-INF/resources/frontend/connector.js(1 hunks)src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownEditorDemo.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-vaadin24
🔇 Additional comments (6)
pom.xml (1)
7-7: Version update correctly reflects the new feature additionThe version increment from 1.1.1-SNAPSHOT to 1.2.0-SNAPSHOT follows semantic versioning principles, appropriately indicating a new feature addition (theme support) while maintaining backward compatibility.
src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownEditorDemo.java (1)
24-25: Well-structured import additionsThe new imports for UI and Lumo theme constants support the theme-based approach being implemented.
Also applies to: 32-32
src/main/resources/META-INF/resources/frontend/connector.js (1)
1-74: Well-implemented theme observation connectorThe connector provides a clean implementation for observing theme changes and updating the component accordingly. The code:
- Properly handles initialization checks to prevent multiple observers
- Checks for themes at the document root level first
- Falls back to body-level theme observation with MutationObserver
- Includes proper cleanup in the unobserve method
This implementation follows best practices for frontend connectors in Vaadin.
src/main/java/com/flowingcode/vaadin/addons/markdown/BaseMarkdownComponent.java (3)
22-26: Appropriate import additions and JavaScript dependencyThe new imports and JavaScript annotation properly set up the connection to the frontend connector.
Also applies to: 40-40
124-127: Well-structured utility method for client response timingThe
runBeforeClientResponsemethod provides a clean abstraction for executing JavaScript at the appropriate time in the component lifecycle.
129-145: Excellent implementation of theme observation lifecycleThe overridden
onAttachandonDetachmethods properly manage the theme observation lifecycle:
onAttachinitializes theme observation when the component is added to the UIonDetachcleans up by removing the observer when the component is removedThis prevents memory leaks and ensures the component responds appropriately to theme changes throughout its lifecycle.
There was a problem hiding this comment.
I've tested this and it seems that is not working correctly:
- The idea of the combobox for selecting the variant, was to show how the component looks and not to change the theme variant of the page (for that there is a specific combobox)
- If I change the theme variant by using the checkbox it is not affecting the component, and that was the idea behind the issue ("If the dark mode theme variant is applied to the current page, it should affect also the color mode support of the component.")
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/flowingcode/vaadin/addons/markdown/BaseMarkdownComponent.java (1)
97-104: Potential double subscription after detach/attach cycle
observeThemeChangeis invoked every time the component is attached, butunobserveThemeChangeis called only when the component is detached from the current UI.
If the element is moved between UIs (admittedly rare) or re-attached before the previousbeforeClientResponsehas fired, you could end up with multiple observers.Not critical for most apps, yet you might want to:
- Make
observeThemeChangeidempotent in the JS connector, or- Keep a flag on the client element indicating an active observer.
Just a heads-up for future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/markdown/BaseMarkdownComponent.java(3 hunks)src/main/java/com/flowingcode/vaadin/addons/markdown/MarkdownViewer.java(0 hunks)src/main/resources/META-INF/resources/frontend/connector.js(1 hunks)src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownEditorDemo.java(2 hunks)src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownViewerDemo.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/flowingcode/vaadin/addons/markdown/MarkdownViewer.java
✅ Files skipped from review due to trivial changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownEditorDemo.java
- src/main/resources/META-INF/resources/frontend/connector.js
🔇 Additional comments (3)
src/test/java/com/flowingcode/vaadin/addons/markdown/MarkdownViewerDemo.java (1)
33-37: Removal of manual color-mode controls aligns the demo with new automatic theming – looks goodThe viewer is now added to the layout without the
ComboBoxthat was previously used to pick a mode, which is exactly what the new connector takes care of automatically. Nothing else is required here.Also applies to: 202-203
src/main/java/com/flowingcode/vaadin/addons/markdown/BaseMarkdownComponent.java (2)
40-41: Verify that the static resource path is correct for all build modes
@JavaScript("./connector.js")assumes that the file is placed in the root of the frontend folder (resolved asfrontend://connector.js).
If the file really lives underMETA-INF/resources/frontend/connector.js(as mentioned in the PR description), Vaadin will also find it, but only when it is copied during the prepare-frontend phase. In production builds the path occasionally breaks if the file is relocated or renamed.
Please double-check that:
mvn -Pproductioncan still locate the script.- The jar classifier (if any) does not strip the resource.
An explicit path such as
@JavaScript("frontend://connector.js")can make the intent clearer.
83-86: Race-condition safeguard: schedule JS only when UI is attachedGood idea to wrap
beforeClientResponseinrunWhenAttached; this prevents NPEs during lazy attach.
| protected void onAttach(AttachEvent attachEvent) { | ||
| super.onAttach(attachEvent); | ||
|
|
||
| runBeforeClientResponse(ui -> ui.getPage().executeJs( | ||
| "window.Vaadin.Flow.fcMarkdownEditorConnector.observeThemeChange($0)", | ||
| getElement())); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing connector on first paint
ui.getPage().executeJs("window.Vaadin.Flow.fcMarkdownEditorConnector.observeThemeChange($0)", …) will throw a client-side error if the connector script has not yet been executed (e.g., if the bundle is still downloading).
Consider adding a defensive check:
- "window.Vaadin.Flow.fcMarkdownEditorConnector.observeThemeChange($0)"
+ "window.Vaadin && Vaadin.Flow && Vaadin.Flow.fcMarkdownEditorConnector "
+ + "? Vaadin.Flow.fcMarkdownEditorConnector.observeThemeChange($0) : undefined"or wrap the call in the JS connector itself.
Committable suggestion skipped: line range outside the PR's diff.
Close #10
Summary by CodeRabbit