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.

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

Fix Card to be backwards compatible #5762

Merged
merged 5 commits into from
Nov 2, 2021
Merged

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Oct 28, 2021

What does this PR do?

In a recent PR, we introduced changes that were not backwards compatible. The issue was rooted in a side-effect of styled components forwardedAs prop and how it handles creating classes for styled HOCs.

Essentially, by abstracting Card to go from being just a Box to a styled(Box), we introduce another layer of styled components which resulted in the Box props applied to a Card getting lost. Styled components isn't set up to know how to merge/layer so many levels of chained styled components. Suddenly when we updated the DS site, everything looked like this (yikes):

Screen Shot 2021-10-28 at 3 26 58 PM

Therefore, this PR takes a new approach where the desired styling for Card is passed as kind on Box. Similar to what we do with Button. kind takes a structure of { hover: theme.card.hover.container, ...theme.card.container}. Then, StyledBox handles the kind styles.

Where should the reviewer start?

src/js/components/Card/Card.js

What testing has been done on this PR?

Tested on Clickable story.

Also, I mirrored what the setup in the DS site was (which caused backwards compatibility issues) and confirmed this would work correctly:

// StyledConentCard (as it existed in DS)
const StyledNewCard = styled(Card)`
  &:hover {
    background: lavender;
  }
  transition: all 0.2s ease-in-out;
`;

const theme = deepMerge(grommet, {
  card: {
    hover: {
      container: {
        elevation: 'large',
      },
    },
  },
});
export const Simple = () => (
  <Grommet theme={theme}>
    <Box pad="medium" gap="medium">
      <Link href="/" passHref>
        <StyledNewCard
          width="medium"
          forwardedAs="a"
          background="gold"
          pad="25px"
          style={{ textDecoration: 'none' }}
        >
          This is equivalent to what we have as ContentCard in Design System.
        </StyledNewCard>
      </Link>
    </Box>
  </Grommet>
);
export default {
  title: 'Layout/Card/Simple',
};
Screen.Recording.2021-10-28.at.3.13.06.PM.mov

How should this be manually tested?

Created a styled component that is a styled(Card) and pass Box props on the card. All of those styles should appear.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes, need to move the extend outside of the hover block.

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Backwards compatible.

@taysea taysea added this to In progress in Grommet/Design System Backlog Nov 1, 2021
@taysea taysea self-assigned this Nov 1, 2021
@taysea taysea moved this from In progress to In Review in Grommet/Design System Backlog Nov 1, 2021
@ericsoderberghp ericsoderberghp merged commit 7be9108 into grommet:master Nov 2, 2021
Grommet/Design System Backlog automation moved this from In Review to Done Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants