#34195 Scrollbars hide gantt content#103
#34195 Scrollbars hide gantt content#103daniela-mateescu wants to merge 3 commits intomaster-flower-platformfrom
Conversation
Summary of ChangesHello @daniela-mateescu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where scrollbars were obscuring content within the Gantt chart. The solution involves a slight increase in scrollbar dimensions, a modernization of scrollbar arrow styling using inline SVG data URIs for better maintainability and performance, and a crucial adjustment to the timeline's maximum date calculation to prevent content from being hidden by the vertical scrollbar. These changes collectively enhance the visibility and usability of the Gantt chart interface. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where scrollbars were obscuring content in the Gantt chart. The core of the fix is in src/timeline.js, where the timeline's maximum date is now extended to account for the width of the vertical scrollbar, preventing content from being hidden. Additionally, the scrollbar styling has been significantly improved in src/style.css by replacing PNG image assets with inline SVGs, which is a great enhancement for performance and scalability. The scrollbar size has also been adjusted. My review includes a high-priority comment regarding the fragility of the scrollbar width calculation and several medium-priority suggestions to improve the new CSS for better consistency and maintainability.
src/timeline.js
Outdated
There was a problem hiding this comment.
The calculation of vScrollbarWidth relies on the internal DOM structure of the react-virtualized Grid component (this._gridDomNode.firstChild). This approach is fragile and could break if react-virtualized changes its internal structure in a future update. While this might be the only way to get the scrollbar width, it would be beneficial to add a comment explaining this dependency and the potential risk. For example: // This calculation is fragile as it depends on the internal DOM structure of react-virtualized's Grid.
For better readability, you could also consider extracting this._gridDomNode.firstChild into a variable to avoid accessing it twice.
|
|
||
| .rct9k-horizontal-scrollbar-inner { | ||
| height: 1px; | ||
| padding-top: 2px; |
There was a problem hiding this comment.
The padding-top: 2px; appears to be a "magic number". To improve code maintainability, please add a comment explaining the purpose of this padding. For instance, is it for visual alignment with another element?
src/style.css
Outdated
There was a problem hiding this comment.
The width and height of the scrollbar button are set to 15px, while the background-size is 16px 16px and the SCROLLBAR_SIZE constant was updated to 16. This inconsistency may cause the background image to be clipped. For consistency and to prevent potential rendering issues, it's recommended to set the width and height to 16px.
| width: 15px; | |
| height: 15px; | |
| width: 16px; | |
| height: 16px; |
src/style.css
Outdated
There was a problem hiding this comment.
The width and height of the scrollbar button are set to 15px, while the background-size is 16px 16px and the SCROLLBAR_SIZE constant is 16. This inconsistency may cause the background image to be clipped. For consistency and to prevent potential rendering issues, it's recommended to set the width and height to 16px.
| width: 15px; | |
| height: 15px; | |
| width: 16px; | |
| height: 16px; |
src/style.css
Outdated
There was a problem hiding this comment.
The width and height of the scrollbar button are set to 15px, while the background-size is 16px 16px and the SCROLLBAR_SIZE constant is 16. This inconsistency may cause the background image to be clipped. For consistency and to prevent potential rendering issues, it's recommended to set the width and height to 16px.
| width: 15px; | |
| height: 15px; | |
| width: 15px; | |
| height: 16px; |
src/style.css
Outdated
There was a problem hiding this comment.
The width and height of the scrollbar button are set to 15px, while the background-size is 16px 16px and the SCROLLBAR_SIZE constant is 16. This inconsistency may cause the background image to be clipped. For consistency and to prevent potential rendering issues, it's recommended to set the width and height to 16px.
| width: 15px; | |
| height: 15px; | |
| width: 16px; | |
| height: 16px; |
2 participants
Proposed Change:
What is your change
Change type
Put an
xin the boxes that applyChecklist