chore: chat widget polish fixes #37124
Conversation
WalkthroughThe pull request introduces various modifications across multiple components in the design system. Key changes include updates to CSS selectors for segmented controls, enhancements to input styles, and refinements to the Markdown component. Notably, the 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
|
github-actions
bot
added
the
skip-changelog
| @@ -77,7 +77,9 @@ export const StyledControlContainer = styled.div` | |||
|
|
|||
| /* Select all segments which is not a selected and last child */ | |||
| /* seperator */ | |||
| &:not(:last-child):not([data-selected="true"]):after { | |||
| &:not(:last-child):not([data-selected="true"]):not( | |||
There was a problem hiding this comment.
This is done to hide seperator on left of current selected element as well.
cc: @albinAppsmith
| @@ -101,7 +101,7 @@ | |||
| ); | |||
| --icon-offset: calc((var(--input-height) - var(--icon-size)) / 2); | |||
|
|
|||
| bottom: var(--icon-offset); | |||
| bottom: round(up, var(--icon-offset), 0.5px); | |||
There was a problem hiding this comment.
The suffix button was not properly aligned vertically due to rounding of pixel values. with round of CSS, it looks better now.
There was a problem hiding this comment.
Could you take a before/after screenshot?
There was a problem hiding this comment.
@KelvinOm It's 0.5 pixel diff. Hard to notice tbh.
There was a problem hiding this comment.
@jsartisan It seems that there is something wrong with sizes here. Flex should do the alignment for us without additional CSS. Anyway, if it works, let's move on.
| <code {...rest} className={className}> | ||
| {children} | ||
| </code> | ||
| <code className={className}>{children}</code> |
There was a problem hiding this comment.
this is done to avoid passing the of whole node object to code dom element.
| @@ -95,6 +95,12 @@ | |||
| overflow: auto; | |||
| } | |||
|
|
|||
| code:not(pre code) { | |||
There was a problem hiding this comment.
This is the styling of inline code. added not(pre code) to skip the code snippets.
| @@ -12,8 +12,16 @@ import { | |||
| } from "utils/AppsmithUtils"; | |||
|
|
|||
| const StyledSegmentedControl = styled(SegmentedControl)` | |||
| &.ads-v2-segmented-control { | |||
There was a problem hiding this comment.
These styles updates are done so that 4 buttons variants can fit in.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/Container.tsx (1)
46-46: Consider documenting the noPadding prop.
While the implementation is correct, adding JSDoc comments would help maintain consistency with the existing documentation style seen above for other props.
elevatedBackground: boolean;
+ /** When true, removes padding from the container */
noPadding?: boolean;app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1)
80-82: Consider adding visual regression test coverage
This styling change affects the visual appearance of segment separators. Ensure it's covered by visual regression tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1 hunks)
- app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/Markdown/src/mdComponents/Code.tsx (3 hunks)
- app/client/packages/design-system/widgets/src/components/Markdown/src/styles.module.css (1 hunks)
- app/client/packages/design-system/widgets/src/components/Markdown/stories/Markdown.stories.ts (1 hunks)
- app/client/src/components/propertyControls/IconTabControl.tsx (1 hunks)
- app/client/src/layoutSystems/anvil/common/styles.module.css (1 hunks)
- app/client/src/modules/ui-builder/ui/wds/Container.tsx (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/design-system/widgets/src/components/Markdown/stories/Markdown.stories.ts
🔇 Additional comments (8)
app/client/src/modules/ui-builder/ui/wds/Container.tsx (1)
32-32: LGTM: Clean data attribute implementation.
The data attribute follows HTML conventions and aligns with the existing pattern.
app/client/packages/design-system/widgets/src/components/Markdown/src/mdComponents/Code.tsx (3)
2-2: LGTM! Import ordering looks good.
16-16: Good cleanup! Explicit props destructuring is cleaner than spreading unknown props.
53-53: 🛠️ Refactor suggestion
Consider adding accessibility attributes.
The code element could benefit from ARIA attributes for better screen reader support.
Consider adding language information for accessibility:
- <code className={className}>{children}</code>
+ <code className={className} lang={match?.[1]} aria-label={`Code snippet in ${match?.[1] || 'text'}`}>{children}</code>Likely invalid or redundant comment.
app/client/packages/design-system/widgets/src/components/Markdown/src/styles.module.css (1)
98-102: LGTM! Well-structured inline code styling.
The new CSS rule effectively differentiates inline code from code blocks while maintaining design system consistency.
app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx (1)
80-82: Check browser compatibility for :has() selector
The selector enhancement improves the visual polish by preventing double borders. However, the :has() selector has limited browser support in older versions.
✅ Verification successful
:has() selector is already used extensively in the codebase with modern build tooling
The :has() selector is already widely used throughout the codebase, particularly in design system components and layout systems. The project uses modern build tools (PostCSS, webpack) that can handle this selector. The browserlist configuration (>0.2%, not dead, not ie <= 11) also indicates support for modern CSS features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any explicit browser support definition
rg -l "browserlist|browserslist|@support" --type css --type json
Length of output: 145
Script:
#!/bin/bash
# Check browserlist configuration in package.json files
cat app/client/package.json | jq -r '.browserslist? // empty'
cat app/client/packages/design-system/ads-old/package.json | jq -r '.browserslist? // empty'
# Check if :has selector is used elsewhere in the codebase
rg ":has\(" --type css --type tsx -A 2
Length of output: 361
Script:
#!/bin/bash
# Check if :has selector is used elsewhere in the codebase with correct file types
rg ":has\(" --type-add 'tsx:*.tsx' --type tsx --type css -A 2
# Check package.json for build/transpilation tools that might handle modern CSS
cat app/client/package.json | jq '.dependencies + .devDependencies | keys' 2>/dev/null
Length of output: 112218
app/client/src/layoutSystems/anvil/common/styles.module.css (2)
10-12: LGTM! Clean implementation of conditional padding.
The addition of :not([data-no-padding]) provides a clean way to override padding when needed while maintaining the existing elevation styles.
14-18: Verify browser compatibility for the :has() selector.
While the implementation is logically sound, the :has() selector used in complex combinations might have browser compatibility issues.
Consider adding a fallback style or documenting minimum browser version requirements if not already present.
✅ Verification successful
:has() selector usage is safe with current browser requirements
The codebase's browserslist configuration (>0.2%, not dead, not ie <= 11, not op_mini all) supports modern browsers where :has() selector has wide adoption. The implementation is properly used across multiple elevation scenarios with clear documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any browser compatibility comments or configurations
rg -l "browser.*compatibility|@supports|:has|browserslist" --type css
rg -l "browser.*compatibility|@supports|:has|browserslist" package.json
Length of output: 985
Script:
#!/bin/bash
# Let's check the content of the found CSS files for specific :has() usage and browser compatibility comments
rg -A 2 -B 2 ":has|@supports|browser.*compatibility" "app/client/src/layoutSystems/anvil/common/styles.module.css"
# Check package.json in the client directory for browserslist config
cat app/client/package.json | grep -A 5 -B 5 "browserslist"
# Check if there's a separate browserslist config file
fd -t f "^\.browserslistrc$|^browserslist$"
Length of output: 2578
| &.ads-v2-segmented-control { | ||
| gap: 0; | ||
| } | ||
|
|
||
| > .ads-v2-segmented-control__segments-container { | ||
| flex: 1 1 0%; | ||
| flex: 1 1 auto; | ||
| } | ||
|
|
||
| > .ads-v2-segmented-control__segments-container:has(.ads-v2-text) span { | ||
| padding: 0; |
There was a problem hiding this comment.
💡 Codebase verification
CSS changes need adjustment to maintain consistency
The changes in IconTabControl override base styles in a way that could break consistency with other components. Specifically:
- The base SegmentedControl has a default gap of
var(--ads-v2-spaces-2)which shouldn't be removed - Other components (MainContainerWidthToggles, ButtonGroupSetting) use
flex: 1 1 0%while this change usesflex: 1 1 auto
Please update the CSS to:
- Keep the base component's gap or use the design system variable
- Match the flex property with other implementations (
flex: 1 1 0%) - Document why padding needs to be removed for text segments if it's an intentional exception
🔗 Analysis chain
LGTM, but please verify visual alignment
The CSS changes look good and align with the chat widget polish objectives. The flex and padding adjustments should improve the visual consistency.
Let's verify if these changes affect other components using SegmentedControl:
Let me search for both the component name and the CSS class to understand its usage across the codebase. The previous command failed because the file type flag wasn't supported, so I've modified the approach to be more inclusive.
I need to verify if these CSS changes in IconTabControl are overriding any base styles and how they might affect other components using the same class names.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find other components using SegmentedControl to verify visual impact
rg -l "SegmentedControl" --type tsx
Length of output: 63
Script:
#!/bin/bash
# Find files containing SegmentedControl using ripgrep without file type restriction
rg -l "SegmentedControl"
# Also search for the specific class name used in the CSS
rg -l "ads-v2-segmented-control"
Length of output: 3650
Script:
#!/bin/bash
# Let's check the base SegmentedControl component to understand the default styling
cat app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx
# Also check other components that might be affected by these changes
cat app/client/src/pages/Editor/MainContainerWidthToggles.tsx
cat app/client/src/pages/Editor/AppSettingsPane/AppSettings/NavigationSettings/ButtonGroupSetting.tsx
Length of output: 7118
| @@ -101,7 +101,7 @@ | |||
| ); | |||
| --icon-offset: calc((var(--input-height) - var(--icon-size)) / 2); | |||
|
|
|||
| bottom: var(--icon-offset); | |||
| bottom: round(up, var(--icon-offset), 0.5px); | |||
There was a problem hiding this comment.
💡 Codebase verification
Add fallback for browsers that don't support CSS round() function
The CSS round() function has limited browser support (Chrome 117+, Edge 117+, Safari 16.4+). Since no fallback is implemented and the codebase doesn't use autoprefixer, this could cause layout issues in unsupported browsers.
- Add a fallback value before using the
round()function:
bottom: var(--icon-offset);
bottom: round(up, var(--icon-offset), 0.5px);🔗 Analysis chain
Verify browser compatibility for CSS round() function.
The use of round() function to prevent sub-pixel rendering issues is a good improvement for visual polish.
Let's verify browser support for the CSS round() function:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any fallback or @supports rule for browsers that don't support CSS round()
rg -i "(@supports|\bround\b.*\{)" app/client/packages/design-system
# Check package.json for any CSS polyfill dependencies
rg -g "package.json" -i "postcss|autoprefixer"
Length of output: 568
| @@ -101,7 +101,7 @@ | |||
| ); | |||
| --icon-offset: calc((var(--input-height) - var(--icon-size)) / 2); | |||
|
|
|||
| bottom: var(--icon-offset); | |||
| bottom: round(up, var(--icon-offset), 0.5px); | |||
There was a problem hiding this comment.
Could you take a before/after screenshot?
- [x] Fix button variant control in the property pane appsmithorg#37005 - [x] elevatedBackground for chat should work the same as for statbox - [x] Markdown polishing. We need to fix lists, inline code, etc. Check how it works with a table. /ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a new `noPadding` property for the `ContainerComponent`, allowing for conditional styling. - Enhanced styling for segmented controls and input components for improved visual clarity and responsiveness. - **Bug Fixes** - Refined CSS selectors for separators in segmented controls to improve visual representation. - **Style** - Adjusted styles for input groups and inline code elements for better alignment and visual feedback. - **Documentation** - Updated Storybook configuration for the `Markdown` component to reflect new formatting changes. <!-- 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/11571964148> > Commit: 29bac9b > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11571964148&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Tue, 29 Oct 2024 10:51:44 UTC <!-- end of auto-generated comment: Cypress test results -->
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
Release Notes
New Features
noPaddingproperty for theContainerComponent, allowing for conditional styling.Bug Fixes
Style
Documentation
Markdowncomponent to reflect new formatting changes.Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11571964148
Commit: 29bac9b
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Tue, 29 Oct 2024 10:51:44 UTC