-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add ResizeObserver to change textarea height based on width changes #38011
Conversation
WalkthroughThe pull request includes updates to several files in the design system. A new dependency Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (6)
app/client/packages/design-system/widgets/package.json
(1 hunks)app/client/packages/design-system/widgets/src/components/Input/src/TextAreaInput.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/Input/src/types.ts
(1 hunks)app/client/packages/design-system/widgets/src/components/TextArea/src/TextArea.tsx
(5 hunks)app/client/packages/design-system/widgets/src/components/TextArea/src/types.ts
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/packages/design-system/widgets/src/components/TextArea/src/TextArea.tsx
[error] 117-117: Avoid redundant Boolean
call
It is not necessary to use Boolean
call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean
call
(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (5)
app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (2)
24-25
: Verify overflow behavior with input states
The overflow: hidden
might interact with various input states. Please verify that this doesn't affect:
- Focused state with long content
- Invalid state with error messages
- Readonly state with text selection
Consider documenting these edge cases in the component's test suite.
24-25
: Clarify the timeline for implementing max height
The comment indicates that overflow: hidden
is temporary. Please provide more context:
- When will the max height be implemented?
- Is there a tracking issue for this?
This property might affect the ResizeObserver's ability to calculate correct heights.
✅ Verification successful
Let me gather more context about the textarea implementation and ResizeObserver usage.
Let me search for any ResizeObserver implementation in the Input component to understand the context better.
Let me check the Input component implementation and any related PRs to understand the context of this overflow property.
Let me check the PR description and related files to understand the context of the ResizeObserver implementation.
Based on the findings, I can now provide a final response:
The overflow: hidden
property should be removed now
The TextArea component already implements ResizeObserver (via useResizeObserver
from usehooks-ts) for height adjustments, as seen in the TextArea.tsx implementation. The overflow property was likely a temporary solution before this implementation.
• Remove the overflow property and its associated comment since the proper height adjustment mechanism is now in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any max-height implementations or TODOs in related files
rg -g '*.{ts,tsx,css}' -i 'max-?height|TODO.*height' app/client/packages/design-system/widgets/src/
Length of output: 828
Script:
#!/bin/bash
# Search for ResizeObserver implementation and textarea height adjustments
rg -g '*.{ts,tsx}' -B 2 -A 2 'ResizeObserver|textarea.*height' app/client/packages/design-system/widgets/src/components/Input/
# Check if there are any related PRs or issues mentioned in recent commits
git log --oneline -n 5 -- app/client/packages/design-system/widgets/src/components/Input/
# Look for any height-related props or configurations in Input component
ast-grep --pattern 'interface $_ {
$$$
height
$$$
}'
Length of output: 65786
Script:
#!/bin/bash
# Search for ResizeObserver and height-related code in Input component
rg -g '*.{ts,tsx}' -B 2 -A 2 'ResizeObserver|height.*adjust|autoHeight' app/client/packages/design-system/widgets/src/components/Input/
# Check recent changes to the Input component
git log -p -n 1 app/client/packages/design-system/widgets/src/components/Input/
Length of output: 137
Script:
#!/bin/bash
# Look for the Input component implementation and any height-related code
cat app/client/packages/design-system/widgets/src/components/Input/src/Input.tsx
# Search for any related PRs or issues mentioning textarea height
rg -g '*.{ts,tsx,css}' -B 2 -A 2 'textarea.*height|max-height' app/client/packages/design-system/widgets/src/components/Input/
Length of output: 2119
Script:
#!/bin/bash
# Check for any TextArea component that might be using this Input component
rg -g '*.{ts,tsx,css}' -B 2 -A 2 'TextArea.*Input|ResizeObserver' app/client/packages/design-system/widgets/src/
# Look for any recent changes or PRs related to TextArea height adjustments
git log --grep="textarea\|height\|resize" -n 3 app/client/packages/design-system/widgets/src/components/Input/
Length of output: 8309
app/client/packages/design-system/widgets/src/components/TextArea/src/types.ts (1)
Line range hint 1-13
: Enhancements to TextAreaProps
are appropriate
The additions of fieldClassName
, inputClassName
, and size
properties enhance the customization options of the TextArea
component.
app/client/packages/design-system/widgets/src/components/Input/src/types.ts (1)
24-24
: Addition of className
prop is suitable
Including the className
prop in TextAreaInputProps
allows for greater styling flexibility.
app/client/packages/design-system/widgets/src/components/Input/src/TextAreaInput.tsx (1)
15-15
: Inclusion of className
prop is appropriate
Adding className
to the component props enhances the ability to apply custom styles.
@@ -30,7 +30,8 @@ | |||
"react-markdown": "^9.0.1", | |||
"react-syntax-highlighter": "^15.5.0", | |||
"react-transition-group": "^4.4.5", | |||
"remark-gfm": "^4.0.0" | |||
"remark-gfm": "^4.0.0", | |||
"usehooks-ts": "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Pin usehooks-ts to version ^3.1.0
The package is already used in the root app/client/package.json
with version ^3.1.0
. Use the same version to maintain consistency across the codebase.
🔗 Analysis chain
Pin the version of usehooks-ts
Using "*" for version can lead to unexpected breaking changes in production. Please specify a fixed version.
Let's check the latest stable version and recent updates:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest version and update frequency
curl -s https://registry.npmjs.org/usehooks-ts | jq -r '.["dist-tags"].latest, .time | to_entries | .[] | select(.key|test("^[0-9]")) | .key + " -> " + .value'
Length of output: 216
Script:
#!/bin/bash
# Let's try a simpler approach to check the package info
npm view usehooks-ts version
# Also check if this package is already used elsewhere in the codebase
rg "\"usehooks-ts\"" -g "package.json"
# Check for existing ResizeObserver usage
rg "ResizeObserver" -A 2 -B 2
Length of output: 26447
💡 Codebase verification
Based on the earlier results, I can generate the final response:
Native ResizeObserver would reduce bundle size by ~17.5KB
The package usehooks-ts
adds 17.9KB (5.4KB gzipped) to the bundle. From the codebase scan, we found:
useResizeObserver
anduseDebounceCallback
are the only hooks being used- Used in 4 components: TextArea, ToolbarButtons, InlineButtons, and GroupOrientation hook
🔗 Analysis chain
Consider using native ResizeObserver
The usehooks-ts
package is being added primarily for ResizeObserver functionality. Consider using the native ResizeObserver API which has good browser support.
Let's check the bundle size impact:
Let me check if the package is being used for other hooks besides ResizeObserver.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check bundle size impact of usehooks-ts
# Get package size
curl -s https://bundlephobia.com/api/size?package=usehooks-ts | jq '.gzip, .size'
# Check if package is being used only for ResizeObserver
rg -l "from 'usehooks-ts'" | xargs rg "useResizeObserver"
Length of output: 1083
Script:
#!/bin/bash
# Check all imports from usehooks-ts to see what other hooks are being used
rg "from 'usehooks-ts'" -A 1
Length of output: 36
Script:
#!/bin/bash
# Search for all imports from usehooks-ts package
rg "import.*from 'usehooks-ts'" --type ts --type tsx
Length of output: 88
Script:
#!/bin/bash
# Search for all imports from usehooks-ts package
rg "import.*from 'usehooks-ts'"
Length of output: 39
const [textFieldHeight, setTextFieldHeightHeight] = useState<number | null>( | ||
null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the typo in the state setter function name
The state setter function setTextFieldHeightHeight
is misspelled. It should be setTextFieldHeight
.
Apply this diff to correct the typo:
const [textFieldHeight, setTextFieldHeightHeight] = useState<number | null>(
null,
);
+const [textFieldHeight, setTextFieldHeight] = useState<number | null>(null);
setTextFieldHeightHeight(height + paddingTop + paddingBottom);
+setTextFieldHeight(height + paddingTop + paddingBottom);
Also applies to: 75-75
const styles = { | ||
// The --input-height it may be useful to align the prefix or suffix. | ||
// Why can't we do this with CSS? Reason is that the height of the input is calculated based on the content. | ||
"--input-height": Boolean(textFieldHeight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Remove the redundant Boolean
cast
The explicit Boolean
cast is unnecessary as textFieldHeight
will be coerced to a boolean in the ternary expression.
Apply this diff to simplify the code:
-"--input-height": Boolean(textFieldHeight) ? `${textFieldHeight}px` : "auto",
+"--input-height": textFieldHeight ? `${textFieldHeight}px` : "auto",
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 117-117: Avoid redundant Boolean
call
It is not necessary to use Boolean
call when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant Boolean
call
(lint/complexity/noExtraBooleanCast)
…ges (appsmithorg#38011) ## Description Moving changes from [EE PR](appsmithorg/appsmith-ee#5652). ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12196767279> > Commit: 35ef576 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12196767279&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 06 Dec 2024 10:54:39 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Added a new dependency to enhance component functionality. - Introduced a `className` prop for the `TextAreaInput` component, allowing for greater styling customization. - Enhanced the `TextArea` component with dynamic height adjustment and additional styling options. - **Bug Fixes** - Streamlined CSS for input elements by removing unnecessary calculations and improving overflow behavior. - **Documentation** - Updated types to include new properties for better flexibility in component usage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Moving changes from EE PR.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12196767279
Commit: 35ef576
Cypress dashboard.
Tags:
@tag.Sanity
Spec:
Fri, 06 Dec 2024 10:54:39 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
className
prop for theTextAreaInput
component, allowing for greater styling customization.TextArea
component with dynamic height adjustment and additional styling options.Bug Fixes
Documentation