chore: shift the chat button when scrollbar shows#39559
Conversation
WalkthroughThis PR enhances the design system by introducing dynamic scrollbar width support. A new utility function computes the scrollbar width, which is then memoized in the Changes
Sequence Diagram(s)sequenceDiagram
participant TP as ThemeProvider
participant H as useCssTokens Hook
participant G as getScrollbarWidth
TP->>H: Call useCssTokens(theme)
H->>G: Compute scrollbar width
G-->>H: Return width value
H-->>TP: Return CSS class with --scrollbar-width property
TP->>DOM: Render component with updated className
sequenceDiagram
participant TA as TextArea Component
participant IE as Input Element
TA->>IE: Adjust height based on content
IE-->>TA: Provide clientHeight and scrollHeight
alt scrollHeight > clientHeight
TA->>IE: Set data-has-scrollbar = "true"
else
TA->>IE: Remove data-has-scrollbar attribute
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![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)
app/client/packages/utils/src/dom/getScrollbarWidth.ts (1)
1-15: Consider memoizing the scrollbar width calculation.The function creates and destroys DOM elements on each call, which is inefficient if called frequently. Scrollbar width rarely changes during a session.
-export const getScrollbarWidth = () => { +export const getScrollbarWidth = (() => { + let width: number | null = null; + + return () => { + if (width !== null) return width; + + // Only calculate if we're in a browser environment + if (typeof document === 'undefined') return 0; + const scrollDiv = document.createElement("div"); scrollDiv.setAttribute( "style", "width: 100px; height: 100px; overflow: scroll; position:absolute; top:-9999px;", ); document.body.appendChild(scrollDiv); const scrollbarWidth = scrollDiv.offsetWidth - scrollDiv.clientWidth; document.body.removeChild(scrollDiv); - return scrollbarWidth; + width = scrollbarWidth; + return width; + }; };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx(2 hunks)app/client/packages/design-system/theming/src/theme/src/ThemeProvider.tsx(2 hunks)app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(1 hunks)app/client/packages/design-system/widgets/src/components/TextArea/src/TextArea.tsx(1 hunks)app/client/packages/utils/src/dom/getScrollbarWidth.ts(1 hunks)app/client/packages/utils/src/dom/index.ts(1 hunks)app/client/packages/utils/src/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/utils/src/dom/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (8)
app/client/packages/utils/src/index.ts (1)
6-6: New DOM utility export looks good.The export statement correctly exposes the DOM utility functions, maintaining consistent structure with other exports.
app/client/packages/design-system/theming/src/theme/src/ThemeProvider.tsx (2)
22-22: Correctly added scrollbarWidth token.The new scrollbarWidthClassName is properly included in the destructuring from useCssTokens.
36-36: Properly applied scrollbarWidth class.The new scrollbarWidthClassName is correctly incorporated into the className composition.
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1)
59-62:Details
✅ Verification successful
CSS fix for suffix positioning with scrollbar present.
This rule correctly adjusts the suffix element's position when a scrollbar appears in a textarea, accomplishing the PR objective.
🏁 Script executed:
#!/bin/bash # Check if this data-has-scrollbar attribute is set somewhere in the codebase # Use AST-grep to locate where this attribute might be set ast-grep --pattern $'[data-has-scrollbar]' || echo "No direct attribute references found" # Check for more implementation details using ripgrep rg -l "data-has-scrollbar" || echo "No string matches found" # Look for JavaScript/TypeScript code that might be setting this attribute rg -l "setAttr.*has-scrollbar|setAttribute.*has-scrollbar" --type ts --type tsx || echo "No attribute setting code found"Length of output: 572
CSS suffix adjustment with scrollbar verified
The CSS rule in
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css(lines 59-62) has been verified. The output confirms that the[data-has-scrollbar]attribute is correctly referenced in the CSS file, and its usage in the TextArea component (app/client/packages/design-system/widgets/src/components/TextArea/src/TextArea.tsx) validates that the rule is applied as intended. This ensures the suffix element is properly repositioned when a scrollbar appears in a textarea.app/client/packages/design-system/widgets/src/components/TextArea/src/TextArea.tsx (1)
80-84: Appropriate handling for scrollbar detection.The implementation correctly detects when a textarea has a scrollbar by comparing scrollHeight against clientHeight, and appropriately sets a data attribute that can be used for styling adjustments.
app/client/packages/design-system/theming/src/hooks/src/useCssTokens.tsx (3)
8-8: LGTM: Clean import addition.Import added correctly for the new functionality.
95-99: Efficient implementation for scrollbar width detection.Good use of useMemo to create the CSS class with scrollbar width. The empty dependency array ensures this is only calculated once, which is appropriate since scrollbar width doesn't change during runtime.
107-107: Effective export of the new functionality.Properly exports the new className for use by consumers of this hook.
| setTextFieldHeight(height + marginTop + marginBottom); | ||
|
|
||
| input.style.height = `${input.scrollHeight}px`; | ||
| input.style.height = `${input.scrollHeight + 1}px`; |
There was a problem hiding this comment.
had to add 1px here, there was a weird scrollbar bug happening without it.
CleanShot.2025-03-05.at.12.28.44.mp4
https://github.com/user-attachments/assets/efccee4b-c329-4766-8fc6-92c8a135f8df /ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced dynamic scrollbar width styling that enhances the overall theme layout. - Input and textarea components now automatically detect and adjust based on scrollbar presence. - Made available a new utility for accurately measuring scrollbar width. - **Style** - Applied new CSS rules to ensure proper positioning of suffix elements in input groups when scrollbars appear. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13673799093> > Commit: a72538b > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13673799093&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Wed, 05 Mar 2025 10:49:39 UTC <!-- end of auto-generated comment: Cypress test results -->
3 participants
CleanShot.2025-03-04.at.22.19.22.mp4
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
New Features
Style
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13673799093
Commit: a72538b
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Wed, 05 Mar 2025 10:49:39 UTC