Skip to content

refactor(Card + FloatingCard)!: changing FloatCard to internal use and refactoring Card#3042

Merged
codecademydev merged 69 commits intomainfrom
kl-gm-925
Mar 31, 2025
Merged

refactor(Card + FloatingCard)!: changing FloatCard to internal use and refactoring Card#3042
codecademydev merged 69 commits intomainfrom
kl-gm-925

Conversation

@LinKCoding
Copy link
Contributor

@LinKCoding LinKCoding commented Feb 21, 2025

Overview

Making FloatingCard an internal component, for Gamut use only.
The styling that FloatingCard provided is now in the Card component, such as:

  • a shadow effect on hover
  • an optional pattern SVG behind the Card

Other changes:

  • FloatingCard is renamed to InternalFloatingCard and marked as deprecated
  • Card now has variants that uses the variant util function
  • Card elements are based off framer motion and provide small animations for the hover effect
  • Card takes on a medium border radius and a shadow hover when isInteractive prop is true (i.e. in a link, or button, or has a clickable area inside the body of the Card)
  • A Card that is not interactive will have a none border radius by default

PR Checklist

Testing Instructions

  1. Go to the Card story
  2. Look over the examples see that examples make sense and work, esp the isInteractive section
  3. Look over the InternalFloatingCard story - notice the callout and the status of "Old Style".
  4. Look over components that use InternalFloatingCard, like Modal, Toast and see that they still work and are ok.
  5. ...
  6. Profit?!

PR Links and Envs

Repository PR Link PR Env
Monolith Monolith PR Monolith Env
Portal App Monorepo Link Portal Env
LE Preview Monorepo Link LE Env
Brand Monorepo Link Storybook

@nx-cloud
Copy link

nx-cloud bot commented Feb 21, 2025

View your CI Pipeline Execution ↗ for commit 6e79a7f.


☁️ Nx Cloud last updated this comment at 2025-03-31 14:06:02 UTC

@LinKCoding LinKCoding changed the title refactor(FloatingCard): changing FloatCard to internal use and refactoring Card refactor(Card + FloatingCard)!: changing FloatCard to internal use and refactoring Card Mar 19, 2025
@LinKCoding LinKCoding marked this pull request as ready for review March 20, 2025 19:02
@LinKCoding LinKCoding requested a review from a team as a code owner March 20, 2025 19:02
Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

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

this is looking awesome - a couple of nits but nothing blocking or necessary. i still need to test mono + monolith but i think this is shipshape already!

Comment on lines 128 to 169
export const ShadowPatternLeft: Story = {
render: () => (
<FlexBox column>
<FlexBox p={24} border={1}>
<Box flexGrow={1}>
<Card shadow="patternLeft">Default with patternLeft</Card>
</Box>
</FlexBox>
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Card variant="white" shadow="patternLeft">
White with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="yellow" shadow="patternLeft">
Yellow with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="beige" shadow="patternLeft">
Beige with patternLeft
</Card>
</Box>
</FlexBox>
<Background bg="navy">
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Card variant="navy" shadow="patternLeft">
Navy with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="hyper" shadow="patternLeft">
Hyper with patternLeft
</Card>
</Box>
</FlexBox>
</Background>
</FlexBox>
),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if these could be DRYed up with a utility React.FC (pass the config and it will generate based on config)

Copy link
Contributor Author

@LinKCoding LinKCoding Mar 25, 2025

Choose a reason for hiding this comment

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

My thought process here was that if I do that, then the "Show Code" button will only show the util comp, e.g.:
<BadgeWithPatternExamples />

And then it wouldn't be as helpful :\

Copy link
Contributor

Choose a reason for hiding this comment

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

true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer I change it tho?
I do agree it's not as DRY as it could be.

@dreamwasp
Copy link
Contributor

image
last thing - should we have focus styles beside the outline?

Copy link
Contributor

@dreamwasp dreamwasp left a comment

Choose a reason for hiding this comment

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

lgtm minus the small visual bugs with dark mode!

Comment on lines 1 to 13
import styled from '@emotion/styled';
import { motion } from 'framer-motion';

import { Box } from '../Box';
import { cardVariants, shadowVariants } from './styles';
import { CardWrapperProps } from './types';

export const MotionBox = motion.create(Box);

export const CardWrapper = styled(MotionBox)<CardWrapperProps>(
cardVariants,
shadowVariants
);
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove the Background card wrapper? everything looks good i'm just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Cass... I remember why I kept Background now...
The outline variant in particular got messed up 🫠
image

the outline color should match the color of the card, but now it's all white.

and I bet the yellow pattern is a result of the pattern getting confused by the background color.


To answer your question tho, I removed it because I forgot all of this and I wanted to get rid of the bg prop that was extra confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the bg prop to

bg={variant !== 'default' ? (variant as Colors) : '' as Colors}

I couldn't cast undefined as Colors :\

is there a better way?

Comment on lines 128 to 169
export const ShadowPatternLeft: Story = {
render: () => (
<FlexBox column>
<FlexBox p={24} border={1}>
<Box flexGrow={1}>
<Card shadow="patternLeft">Default with patternLeft</Card>
</Box>
</FlexBox>
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Card variant="white" shadow="patternLeft">
White with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="yellow" shadow="patternLeft">
Yellow with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="beige" shadow="patternLeft">
Beige with patternLeft
</Card>
</Box>
</FlexBox>
<Background bg="navy">
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Card variant="navy" shadow="patternLeft">
Navy with patternLeft
</Card>
</Box>
<Box flexGrow={1}>
<Card variant="hyper" shadow="patternLeft">
Hyper with patternLeft
</Card>
</Box>
</FlexBox>
</Background>
</FlexBox>
),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

true

Comment on lines 214 to 267
export const IsInteractive: Story = {
render: () => (
<FlexBox column>
<FlexBox p={24} border={1}>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card shadow="patternRight" isInteractive>
Default, patternRight, and isInteractive
</Card>
</Anchor>
</Box>
</FlexBox>
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card variant="white" shadow="none" isInteractive>
White, no shadow, and isInteractive
</Card>
</Anchor>
</Box>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card variant="yellow" shadow="patternLeft" isInteractive>
Yellow, patternLeft, and isInteractive
</Card>
</Anchor>
</Box>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card variant="beige" shadow="outline" isInteractive>
Beige, outline, and isInteractive
</Card>
</Anchor>
</Box>
</FlexBox>
<Background bg="navy">
<FlexBox row gap={16} p={24} border={1}>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card variant="navy" shadow="patternLeft" isInteractive>
Navy, patternLeft, and isInteractive
</Card>
</Anchor>
</Box>
<Box flexGrow={1}>
<Anchor dimensions={1} href="" variant="interface">
<Card variant="hyper" shadow="patternRight" isInteractive>
Hyper, patternRight, and isInteractive
</Card>
</Anchor>
</Box>
</FlexBox>
</Background>
</FlexBox>
Copy link
Contributor

Choose a reason for hiding this comment

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

the colors on dark mode here are a little wonky - maybe we still want a dark border on white and yellow even in dark mode? the pattern on dark mode also turns yellow before disappearing, is that intended?

Screen.Recording.2025-03-28.at.9.26.25.AM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to post a follow-up
onHover, the Card was taking its color from Anchor, which was yellow/hyper
So for the light mode Cards, it probably just wasn't as visible.

setting color="text" on the MotionBox wrapping the Pattern seemed to do the trick :)

Copy link
Contributor

@aresnik11 aresnik11 left a comment

Choose a reason for hiding this comment

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

Looks so good!! Just a couple small notes

animate: {
opacity: 0,
transition: {
duration: timingValues.slow / 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

personally i think this transition is a little slow and prefer it at timingValues.medium but ill defer to stacey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yaaa, I wanted to keep it slower so that not everything just happened at the same time lol
and Stacey was cool with it: https://skillsoftartisan.slack.com/archives/C07BTPZR85A/p1741107708596829?thread_ts=1741096822.725829&cid=C07BTPZR85A

but ya, feels like a good thing to just check with the team on for these kinda updates, esp if we're gonna do more animations in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

image
dmed with stacey on friday, should've asked in gamut-int whoopsy

Copy link
Contributor

Choose a reason for hiding this comment

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

not a sticking point tho just a personal pref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, I'll change then :) thanks

Comment on lines +64 to +74
transition: {
duration: timingValues.fast / 1000,
ease: 'easeOut',
},
},
animate: {
transform: 'translate(4px, -4px)',
boxShadow: `-8px 8px 0 ${theme.colors['shadow-primary']}`,
transition: {
duration: timingValues.fast / 1000,
ease: 'easeIn',
Copy link
Contributor

Choose a reason for hiding this comment

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

these transitions on the non pattern outlines are a little trippy but it also matches what we currently have (checked prod storybook) so i guess its fine, just noting

@codecademydev
Copy link
Collaborator

📬Published Alpha Packages:

@codecademy/gamut@59.3.2-alpha.6e79a7.0
@codecademy/gamut-kit@0.6.489-alpha.6e79a7.0
@codecademy/styleguide@70.6.1-alpha.6e79a7.0

@codecademydev
Copy link
Collaborator

🚀 Styleguide deploy preview ready!

https://67eaa15c6d2adc424ea57f13--gamut-preview.netlify.app

Deploy Logs

Copy link
Contributor

@aresnik11 aresnik11 left a comment

Choose a reason for hiding this comment

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

LGTM!

@LinKCoding LinKCoding added the Ship It :shipit: Automerge this PR when possible label Mar 31, 2025
@codecademydev codecademydev merged commit 0bb9872 into main Mar 31, 2025
21 of 22 checks passed
@codecademydev codecademydev removed the Ship It :shipit: Automerge this PR when possible label Mar 31, 2025
@codecademydev codecademydev deleted the kl-gm-925 branch March 31, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants