Skip to content
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.

Sign up for GitHub

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

chore: chat widget polish fixes #37124

Merged
merged 1 commit into from
Oct 29, 2024
Merged

chore: chat widget polish fixes #37124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/client/packages/design-system/ads/src/SegmentedControl/SegmentedControl.styles.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done to hide seperator on left of current selected element as well.

cc: @albinAppsmith

:has(+ [data-selected="true"])
):after {
content: "";
position: absolute;
right: 0;
Expand Down
2 changes: 1 addition & 1 deletion app/client/packages/design-system/widgets/src/components/Input/src/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suffix button was not properly aligned vertically due to rounding of pixel values. with round of CSS, it looks better now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you take a before/after screenshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before
CleanShot

After
CleanShot 2024-10-29 at 15 54 31

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KelvinOm It's 0.5 pixel diff. Hard to notice tbh.

Copy link
Collaborator

@KelvinOm KelvinOm Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

right: var(--icon-offset);
}

Expand Down
8 changes: 3 additions & 5 deletions app/client/packages/design-system/widgets/src/components/Markdown/src/mdComponents/Code.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Button, Flex, Text } from "@appsmith/wds";
import type { ExtraProps } from "react-markdown";
import { Button, Flex, Text } from "@appsmith/wds";
import React, { useState, useCallback } from "react";
import { useThemeContext } from "@appsmith/wds-theming";
import {
Expand All @@ -13,7 +13,7 @@ type CodeProps = React.ClassAttributes<HTMLElement> &
ExtraProps;

export const Code = (props: CodeProps) => {
const { children, className, ...rest } = props;
const { children, className } = props;
const match = /language-(\w+)/.exec(className ?? "");
const theme = useThemeContext();
const [copied, setCopied] = useState(false);
Expand Down Expand Up @@ -50,9 +50,7 @@ export const Code = (props: CodeProps) => {
</SyntaxHighlighter>
</div>
) : (
<code {...rest} className={className}>
{children}
</code>
<code className={className}>{children}</code>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is done to avoid passing the of whole node object to code dom element.

};

Expand Down
6 changes: 6 additions & 0 deletions app/client/packages/design-system/widgets/src/components/Markdown/src/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@
overflow: auto;
}

code:not(pre code) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the styling of inline code. added not(pre code) to skip the code snippets.

padding-inline: var(--inner-spacing-1);
background: var(--color-bg-accent-subtle);
border-radius: var(--border-radius-elevation-3);
}

pre {
margin-top: var(--inner-spacing-4);
margin-bottom: var(--inner-spacing-4);
Expand Down
2 changes: 1 addition & 1 deletion ...client/packages/design-system/widgets/src/components/Markdown/stories/Markdown.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const Default: Story = {
args: {
children: `# Hello, Markdown!

This is a paragraph with **bold** and *italic* text.
This is a \`paragraph\` with **bold** and *italic* text.

## Code Example

Expand Down
10 changes: 9 additions & 1 deletion app/client/src/components/propertyControls/IconTabControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,16 @@ import {
} from "utils/AppsmithUtils";

const StyledSegmentedControl = styled(SegmentedControl)`
&.ads-v2-segmented-control {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These styles updates are done so that 4 buttons variants can fit in.
CleanShot 2024-10-29 at 15 27 42

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;
Comment on lines +15 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 uses flex: 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

}
`;

Expand Down
12 changes: 6 additions & 6 deletions app/client/src/layoutSystems/anvil/common/styles.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@
*/
.anvilWidgetWrapper {
/** If a section,zone and card have elevation, then add padding */
[data-elevation="true"][elevation="1"],
[data-elevation="true"][elevation="2"],
[data-elevation="true"][elevation="3"],
[data-elevation="true"][elevation="1"]:not([data-no-padding]),
[data-elevation="true"][elevation="2"]:not([data-no-padding]),
[data-elevation="true"][elevation="3"]:not([data-no-padding]),
/** If a section has any zone with elevation, then add padding to all the zones that don't have elevation */
[elevation="1"]:has([elevation="2"][data-elevation="true"]) [elevation="2"][data-elevation="false"],
[elevation="1"]:has([elevation="2"][data-elevation="true"]) [elevation="2"][data-elevation="false"]:not([data-no-padding]),
/** If a section has any card with elevation, then add padding to all the cards that don't have elevation */
[elevation="1"]:has([elevation="3"][data-elevation="true"]) [elevation="3"][data-elevation="false"],
[elevation="1"]:has([elevation="3"][data-elevation="true"]) [elevation="3"][data-elevation="false"]:not([data-no-padding]),
/** If a zone has any card with elevation, then add padding to all the cards that don't have elevation,*/
[elevation="2"]:has([elevation="3"][data-elevation="true"]) [elevation="3"][data-elevation="false"] {
[elevation="2"]:has([elevation="3"][data-elevation="true"]) [elevation="3"][data-elevation="false"]:not([data-no-padding]) {
padding-block: var(--outer-spacing-3);
padding-inline: var(--outer-spacing-3);
}
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/modules/ui-builder/ui/wds/Container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export function ContainerComponent(props: ContainerComponentProps) {
<StyledContainerComponent
className={`${generateClassName(props.widgetId)}`}
data-elevation={props.elevatedBackground}
data-no-padding={props.noPadding}
elevatedBackground={props.elevatedBackground}
elevation={props.elevation}
>
Expand All @@ -42,4 +43,5 @@ export interface ContainerComponentProps {
children?: ReactNode;
elevation: Elevations;
elevatedBackground: boolean;
noPadding?: boolean;
}
Loading