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

Jump to bottom

feat(drag-drop): adds light/dark mode colors #1805

Merged
merged 10 commits into from May 16, 2024
Merged

feat(drag-drop): adds light/dark mode colors #1805

merged 10 commits into from May 16, 2024

Conversation

geotrev
Copy link
Contributor

@geotrev geotrev commented May 9, 2024 •
edited

Description

Colors updated for the following components & stories using new v9 color palette:

  • Draggable
    • Draggable.Grip
  • DraggableList.DropIndicator
  • Dropzone

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@geotrev geotrev self-assigned this May 9, 2024
@coveralls
Copy link

coveralls commented May 9, 2024 •
edited

Coverage Status

coverage: 96.138% (-0.09%) from 96.224%
when pulling 2fda125 on george/drag-drop-v9
into 5282298 on next.

@geotrev geotrev changed the title 🚧 feat(draggable): recolors components with new palette 🚧 feat(drag-drop): recolors components with new palette May 13, 2024
@geotrev geotrev changed the title 🚧 feat(drag-drop): recolors components with new palette feat(drag-drop): recolors components with new palette May 13, 2024
@geotrev geotrev marked this pull request as ready for review May 13, 2024 16:47
@geotrev geotrev requested a review from a team as a code owner May 13, 2024 16:47
@geotrev
Copy link
Contributor Author

geotrev commented May 13, 2024

Waiting for new disabled color variables from #1806

@geotrev geotrev changed the title feat(drag-drop): recolors components with new palette feat(drag-drop): adds light/dark mode colors May 13, 2024
jzempel
jzempel requested changes May 14, 2024
View reviewed changes
packages/drag-drop/demo/~patterns/stories/components.tsx Outdated
@@ -216,6 +218,10 @@ export const DraggableListItem = ({
);
};

const StyledTitle = styled.strong`
color: ${p => getColor({ variable: 'foreground.default', theme: p.theme })};
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Garden never uses bold; should probably be styled with font-weight: ${p => p.theme.fontWeights.semibold};. Also, take care re: semantics of <strong> (add importance to) vs <b> (bring attention to).

Copy link
Contributor Author

@geotrev geotrev May 14, 2024 •
edited

Choose a reason for hiding this comment

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

I also just realized I can convert the former p to a more semantically correct h2, and leave the JSX alone.

Copy link
Member

Choose a reason for hiding this comment

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

[nit] the h2 still has the bold font weight problem. Would it be better to use Garden's <LG tag="h2"> typography component? We just don't want to confuse any consumers that view the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. I'll update.

packages/drag-drop/src/styled/draggable-list/StyledDropIndicator.ts Outdated Show resolved Hide resolved
packages/drag-drop/src/styled/draggable-list/StyledDropIndicator.ts Outdated
@@ -16,15 +16,14 @@ export interface IStyledDropIndicatorProps extends ThemeProps<DefaultTheme> {

const colorStyles = (props: IStyledDropIndicatorProps) => {
const { theme } = props;
const backgroundColor = getColorV8('primaryHue', 600, theme);
const color = getColorV8('primaryHue', 600, theme);
const color = getColor({ variable: 'foreground.primary', theme });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const color = getColor({ variable: 'foreground.primary', theme });
const color = getColor({ variable: 'border.primaryEmphasis', theme });

I'm not seeing this in the designs, but semantically it feels a lot more like a border; conversely, I wouldn't expect this to change if the foreground was modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair. I was teetering between fg and border, anyway. :)

packages/drag-drop/src/styled/draggable/StyledDraggable.ts Outdated
light: { transparency: opacity[200] },
dark: { transparency: opacity[1000] },
theme
}) as string;
Copy link
Member

Choose a reason for hiding this comment

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

Can this as string cast be removed now?

packages/drag-drop/src/styled/draggable/StyledDraggable.ts Outdated
Comment on lines 50 to 55
const placeholderBgColor = getColor({
variable: 'background.emphasis',
transparency: theme.opacity[100],
dark: { offset: -100 },
theme
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const placeholderBgColor = getColor({
variable: 'background.emphasis',
transparency: theme.opacity[100],
dark: { offset: -100 },
theme
});
const placeholderBgColor = getColor({ variable: 'background.disabled', theme });

I think this can be simplified now.

packages/drag-drop/src/styled/draggable/StyledDraggable.ts Outdated
});
const disabledBgColor = getColor({
variable: 'background.disabled',
transparency: theme.opacity[100],
Copy link
Member

Choose a reason for hiding this comment

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

Not needed as transparency is already included with the rgba for background.disabled.

packages/drag-drop/src/styled/dropzone/StyledDropzone.ts Outdated
color = getColorV8('neutralHue', 400, theme);
backgroundColor = getColor({
variable: 'background.disabled',
transparency: theme.opacity[100],
Copy link
Member

Choose a reason for hiding this comment

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

Can remove transparency – already built-in

@steue
Copy link

steue commented May 14, 2024

this is so awesomes, thanks @geotrev. visually looks good to me!

@geotrev @jzempel
fix: use theme.shadows for draggable shadow
6cd2ccb 
Co-authored-by: Jonathan Zempel <jzempel@gmail.com>
@geotrev geotrev requested a review from jzempel May 14, 2024 21:16
@geotrev geotrev merged commit 80ff27a into next May 16, 2024
5 checks passed
@geotrev geotrev deleted the george/drag-drop-v9 branch May 16, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzempel jzempel jzempel approved these changes

Assignees

@geotrev geotrev

Labels
PR: New Feature 🚀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@geotrev @coveralls @steue @jzempel @zendesk-garden