Fix #14556 Removed the extra space occupied by the table.#15360
Fix #14556 Removed the extra space occupied by the table.#15360Siedlerchr merged 14 commits intoJabRef:mainfrom
Conversation
|
Hey @OfficialArpitNege! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
Review Summary by QodoFix WebSearch table excessive whitespace and dynamic sizing
WalkthroughsDescription• Fixed excessive whitespace in WebSearch table layout • Implemented dynamic table height calculation based on content • Added fixed cell size for consistent row rendering • Table now adjusts height when items are added or removed Diagramflowchart LR
A["WebSearchTab"] -->|"setFixedCellSize"| B["Table Row Height"]
A -->|"adjustTableHeight"| C["Dynamic Height Calculation"]
C -->|"header + rows + padding"| D["Optimized Layout"]
E["Item Changes"] -->|"InvalidationListener"| C
File Changes1. jabgui/src/main/java/org/jabref/gui/preferences/websearch/WebSearchTab.java
|
Code Review by Qodo
1.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
…able in WebSearchTab was leaving unwanted space. This commit adjusts the layout to match the design specification.
a2f295b to
6032437
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| // Dynamic height based on font size and number of items | ||
| DoubleBinding rowHeight = Bindings.createDoubleBinding( | ||
| () -> enableWebSearch.getFont() != null ? enableWebSearch.getFont().getSize() * 2.5 : 30.0, |
There was a problem hiding this comment.
Please use constants instead of magic numbers
There was a problem hiding this comment.
Thanks for the review! I’ve replaced the magic numbers with named constants (FONT_HEIGHT_MULTIPLIER, DEFAULT_ROW_HEIGHT, HEADER_HEIGHT_ESTIMATE) and added brief comments to clarify their purpose. The table layout still adapts dynamically to the font size.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hi @calixtus if you have time could you please review this pr , Happy to make any changes if needed , Thanks. |
|
@OfficialArpitNege please don't resolve comments yourselves as we use them for tracking requested changes |
This comment has been minimized.
This comment has been minimized.
| searchEngineTable.fixedCellSizeProperty().bind(rowHeight); | ||
| searchEngineTable.prefHeightProperty().bind( | ||
| Bindings.size(searchEngineTable.getItems()) | ||
| .add(HEADER_HEIGHT_ESTIMATE) // Estimate for header height |
There was a problem hiding this comment.
I have removed the redundant comment and commited the change ! Thanks for the review .
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: 4ee1276 Learn more about TestLens at testlens.app. |
Related issues and pull requests
Closes #14556
PR Description
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)