fix: fix double-click in read-only mode #179
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 1
🧹 Outside diff range and nitpick comments (2)
pom.xml (2)
Line range hint
283-289: New v23 profile looks good.The addition of the v23 profile allows building the project with Vaadin 23.3.5 and Java 11. This provides flexibility for different build configurations.
Consider adding documentation in the project's README or developer guide explaining how to use different profiles and their implications.
Line range hint
291-324: New v24 profile looks good, but consider documenting its status.The v24 profile adds support for Vaadin 24.2.6 with Java 17 and includes necessary changes for Jakarta EE compatibility. This forward-looking approach is commendable.
Given the inclusion of pre-release repositories, it appears that Vaadin 24 might still be in a pre-release state. Consider adding a comment in the pom.xml or documentation to indicate if this profile is experimental or intended for testing purposes only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- pom.xml (1 hunks)
- src/main/java/com/flowingcode/vaadin/addons/twincolgrid/TwinColGrid.java (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
pom.xml (2)
12-12: Vaadin version update looks good.The Vaadin version has been updated from 14.8.20 to 14.11.13. This minor version update is likely to include bug fixes and improvements.
Please ensure you've reviewed the changelog for any breaking changes or new features that might affect your project. You can find the changelog at: https://github.com/vaadin/platform/releases
Line range hint
1-326: Overall structure and consistency of pom.xml is maintained.The changes to the pom.xml file are well-structured and isolated to specific sections (properties and profiles). The main project configuration remains unchanged, which is good for backwards compatibility. The new profiles add flexibility for building with different Vaadin versions without affecting the default build.
src/main/java/com/flowingcode/vaadin/addons/twincolgrid/TwinColGrid.java (3)
153-155: LGTM: Proper initialization of empty data providerThe method
emptyDataProvider()correctly initializes an emptyListDataProviderwith aLinkedHashSetto maintain the order of items.
Line range hint
342-353: Well-structured horizontal container creationThe method
createHorizontalContainer(boolean reverse)is implemented correctly, handling the creation of the horizontal layout based on thereverseparameter.
931-938: Prevent moving items in read-only modeAdding the check
if (!isReadOnly())appropriately prevents items from being moved when the grid is in read-only mode, addressing the issue as intended.
| if (side == available) { | ||
| updateSelection(item, Collections.emptySet(), true); | ||
| } | ||
| if (side == selection) { | ||
| updateSelection(Collections.emptySet(), item, true); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance conditional logic by using 'else if'
To improve clarity and avoid unnecessary checks, consider changing the second if to else if, since side cannot be both available and selection.
Apply this diff to implement the suggestion:
if (side == available) {
updateSelection(item, Collections.emptySet(), true);
}
-if (side == selection) {
+else if (side == selection) {
updateSelection(Collections.emptySet(), item, true);
}📝 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.
| if (side == available) { | |
| updateSelection(item, Collections.emptySet(), true); | |
| } | |
| if (side == selection) { | |
| updateSelection(Collections.emptySet(), item, true); | |
| } | |
| if (side == available) { | |
| updateSelection(item, Collections.emptySet(), true); | |
| } | |
| else if (side == selection) { | |
| updateSelection(Collections.emptySet(), item, true); | |
| } |
Summary by CodeRabbit
New Features
TwinColGridwith improved layout management and item interaction capabilities.Bug Fixes