feat: poc for center align and toolbar shadow #37264
Conversation
WalkthroughThe pull request introduces several modifications across multiple components: 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
|
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11706195006. |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/IDE/Structure/Toolbar.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx
🔇 Additional comments (3)
app/client/src/PluginActionEditor/components/PluginActionForm/components/UQIEditor/UQIEditorForm.tsx (1)
25-31: LGTM! The Flex component changes implement the required features.
The center alignment and scroll container setup look correct. The id addition properly supports the toolbar shadow feature.
Let's verify the scroll behavior integration:
✅ Verification successful
The scroll behavior integration is properly implemented
The Toolbar component in IDE/Structure/Toolbar.tsx correctly uses the uqi-editor-form-content id to detect scrolling and apply shadow effects. The implementation includes proper event cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the scroll behavior integration between UQIEditorForm and Toolbar
# Expected: Find usage of the uqi-editor-form-content id in Toolbar component for scroll detection
# Search for scroll-related logic using the form content id
rg -A 5 "uqi-editor-form-content.*scroll" app/client/src/
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search to find any usage of this id
rg -A 5 "uqi-editor-form-content" app/client/src/
# Also search for any Toolbar component that might use scroll detection
ast-grep --pattern 'const Toolbar = $_ => {
$$$
}'
# Search for scroll-related hooks or utilities that might be used
rg -A 5 "useScroll|handleScroll" app/client/src/
Length of output: 8358
app/client/src/IDE/Structure/Toolbar.tsx (2)
1-1: LGTM: Import changes are correct
38-45: LGTM: Styling changes look good
The transition and positioning styles are well implemented. However, verify that the sticky positioning works correctly with all parent containers.
✅ Verification successful
Let me analyze the sticky positioning concern by checking the parent containers of the Toolbar component.
Let me check how this toolbar is being used in the parent container and if there are any potential positioning conflicts.
Let me check the parent container structure and scroll behavior implementation.
Sticky positioning implementation is correct and safe
The toolbar's sticky positioning works correctly with its parent container:
- Parent
uqi-editor-form-contenthasoverflowY="scroll"which creates a proper scroll context - Toolbar's scroll detection and box-shadow transition are properly implemented
- No conflicting position styles in the parent containers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any potential CSS positioning conflicts in parent containers
rg -l "position:\s*(relative|absolute|fixed)" $(fd -e tsx -e css)
Length of output: 165497
Script:
#!/bin/bash
# Check for any parent containers that might affect sticky positioning
rg -A 5 -B 5 "action-toolbar" app/client/src/IDE/Structure/Toolbar.tsx
Length of output: 697
Script:
#!/bin/bash
# Check how the toolbar is being used in parent containers
rg -A 10 "import.*Toolbar" app/client/src/IDE/Structure/
Length of output: 58
Script:
#!/bin/bash
# Check for the parent container and scroll handling
rg -A 15 "uqi-editor-form-content" app/client/src/
Length of output: 3141
| useEffect(function detectScrollbar() { | ||
| const ele = document.getElementById("uqi-editor-form-content"); | ||
| const toolbar = document.getElementById("action-toolbar"); | ||
|
|
||
| const handleScroll = function () { | ||
| if (ele && ele.scrollTop > 0) { | ||
| toolbar?.style.setProperty( | ||
| "box-shadow", | ||
| "0 4px 6px rgba(0, 0, 0, 0.1)", | ||
| ); | ||
| } else { | ||
| toolbar?.style.setProperty("box-shadow", "none"); | ||
| } | ||
| }; | ||
|
|
||
| if (ele) { | ||
| ele.addEventListener("scroll", handleScroll); | ||
| } | ||
|
|
||
| return function cleanup() { | ||
| ele?.removeEventListener("scroll", handleScroll); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using refs instead of direct DOM queries
The current implementation has several areas for improvement:
- Replace
getElementByIdwith React refs for better reliability and type safety - Extract the shadow value to a constant
- Add null checks for better error handling
+ const toolbarRef = useRef<HTMLDivElement>(null);
+ const contentRef = useRef<HTMLDivElement>(null);
+ const TOOLBAR_SHADOW = "0 4px 6px rgba(0, 0, 0, 0.1)";
useEffect(function detectScrollbar() {
- const ele = document.getElementById("uqi-editor-form-content");
- const toolbar = document.getElementById("action-toolbar");
+ const ele = contentRef.current;
+ const toolbar = toolbarRef.current;
const handleScroll = function () {
- if (ele && ele.scrollTop > 0) {
- toolbar?.style.setProperty(
- "box-shadow",
- "0 4px 6px rgba(0, 0, 0, 0.1)",
- );
+ if (ele?.scrollTop > 0 && toolbar) {
+ toolbar.style.setProperty("box-shadow", TOOLBAR_SHADOW);
} else {
- toolbar?.style.setProperty("box-shadow", "none");
+ toolbar?.style.setProperty("box-shadow", "none");
}
};Committable suggestion skipped: line range outside the PR's diff.
|
Deploy-Preview-URL: https://ce-37264.dp.appsmith.com |
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11718347481. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/IDE/Structure/Toolbar.tsx (1)
41-46: Enhance styling robustnessThe styling changes look good, but consider these improvements:
- Make the transition more specific to avoid unnecessary animations
- Ensure the z-index creates proper stacking context
style={{ - transition: "box-shadow 0.3s ease", + transition: "box-shadow 300ms cubic-bezier(0.4, 0, 0.2, 1)", position: "sticky", top: 0, + backgroundColor: "inherit", // Ensure background is solid for sticky positioning }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/IDE/Structure/Toolbar.tsx(1 hunks)
🔇 Additional comments (1)
app/client/src/IDE/Structure/Toolbar.tsx (1)
9-31: 🛠️ Refactor suggestion
Add error boundaries and extract constants
While the scroll detection logic works, there are a few improvements needed:
- Add null checks before accessing
eleandtoolbar - Extract the shadow value to a constant for reusability
- Consider adding error logging for cases where elements aren't found
+ const TOOLBAR_SHADOW = "0 4px 6px rgba(0, 0, 0, 0.1)";
+
useEffect(function detectScrollbar() {
const ele = document.getElementById("uqi-editor-form-content");
const toolbar = document.getElementById("action-toolbar");
+
+ if (!ele || !toolbar) {
+ console.warn("Required elements not found for toolbar shadow effect");
+ return;
+ }
const handleScroll = function () {
if (ele && ele.scrollTop > 0) {
- toolbar?.style.setProperty(
- "box-shadow",
- "0 4px 6px rgba(0, 0, 0, 0.1)",
- );
+ toolbar.style.setProperty("box-shadow", TOOLBAR_SHADOW);
} else {
- toolbar?.style.setProperty("box-shadow", "none");
+ toolbar.style.setProperty("box-shadow", "none");
}
};Note: Previous review's suggestion about using refs instead of DOM queries is still applicable.
|
Deploy-Preview-URL: https://ce-37264.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Zone/styles.module.css (1)
71-71: Consider creating a tracking issue for the DB config update.The comment indicates this is temporary pending DB configuration updates for Twilio and Airtable integrations.
Would you like me to help create a GitHub issue to track the pending DB configuration updates for Section and Zone in Twilio and Airtable integrations?
app/client/src/components/editorComponents/form/fields/StyledFormComponents.tsx (1)
35-37: Consider using less aggressive word breaking.The
break-word: break-allproperty might break words at unnatural points. Consider usingword-break: break-wordoroverflow-wrap: break-wordfor better readability.max-width: 270px; overflow: hidden; - break-word: break-all; + overflow-wrap: break-word;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Zone/styles.module.css(1 hunks)app/client/src/components/editorComponents/form/fields/StyledFormComponents.tsx(2 hunks)
🔇 Additional comments (2)
app/client/src/PluginActionEditor/components/PluginActionForm/components/ActionForm/Zone/styles.module.css (1)
71-74: LGTM, but note this is a temporary solution.
The change aligns with the file's pattern of removing fixed dimensions for more flexible layouts.
Let's verify if there are any existing issues tracking this temporary change:
app/client/src/components/editorComponents/form/fields/StyledFormComponents.tsx (1)
183-185: LGTM!
The addition of the className prop allows for external styling customization while maintaining the base styled-component styles.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD ac8daad yet
Fri, 08 Nov 2024 11:50:31 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Style