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

fix(card-link): fix to heading style in react #7056

Merged
merged 20 commits into from
Sep 8, 2021
Merged

fix(card-link): fix to heading style in react #7056

merged 20 commits into from
Sep 8, 2021

Conversation

jeffchew
Copy link
Member

@jeffchew jeffchew commented Sep 2, 2021 •
edited
Loading

Related Ticket(s)

No related issue

Description

This addresses an incorrect heading set for <CardLink> in React. It was being set as copy, where it seems that it should be set as heading instead.

This addresses the issue identified in PR #7012:

  1. Card link (React) looks like current heading is using the <p> styling. Should be expressive-heading-02 which is 16px semibold
    image

In addition, web components was using <dds-card-cta>, where it should be using <dds-card-link-cta> instead. Muliple adjustments were made, in addition to documentation and storybook stories.

Changelog

Changed

  • CardLink story setting heading as heading instead of copy.
  • All content block/group components in web components now using <dds-card-link-cta> for the Card link CTA
  • Multiple documentation/storybook fixes

@jeffchew jeffchew added the dev Needs some dev work label Sep 2, 2021
@jeffchew jeffchew added this to the Sprint 21-18 milestone Sep 2, 2021
@jeffchew jeffchew self-assigned this Sep 2, 2021
@jeffchew jeffchew requested a review from a team as a code owner September 2, 2021 17:59
@jeffchew jeffchew mentioned this pull request Sep 2, 2021
Merged
@jeffchew jeffchew added 👀 eyes needed Needs design approval PRs on feature requests and new components have to get design approval before merge. labels Sep 2, 2021
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package Web Components - Codesandbox Examples:
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package Web Components - React Wrapper:
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

@shixiedesign
Copy link
Contributor

shixiedesign commented Sep 2, 2021 •
edited
Loading

Not seeing the React and WC deploy preview links.
Updating branch trying to restart React & web components deploy preview.

A lot of the Card links in Test applications are still wrong but I'm guessing they are a implementation issue of those example pages, and not with our component?

@jeffchew
Copy link
Member Author

jeffchew commented Sep 2, 2021

Not seeing the React and WC deploy preview links.
Updating branch trying to restart React & web components deploy preview.

A lot of the Card links in Test applications are still wrong but I'm guessing they are a implementation issue of those example pages, and not with our component?

Yes, we'd have to go to those components and switch to use the heading instead of copy field.

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 2, 2021 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html

Built with commit: 598ccd766d16d0fcdb2934a094f04a5a007c2f5e

ariellalgilmore
ariellalgilmore approved these changes Sep 2, 2021
View reviewed changes
Copy link
Member

@ariellalgilmore ariellalgilmore left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeffchew jeffchew mentioned this pull request Sep 2, 2021
Merged
@shixiedesign
Copy link
Contributor

shixiedesign commented Sep 3, 2021 •
edited
Loading

React: Now seeing $expressive-heading-03 applied (correct for Card, but not Card link), which is 20-24px depending on breakpoint. For card link, we need $expressive-heading-02, which is 16px semibold.

Current
image

Expecting should be same as what's currently in Web components:
image

@shixiedesign
Copy link
Contributor

shixiedesign commented Sep 8, 2021 •
edited
Loading

  1. Feature section's card link lost the semibold:

image

  1. Content block media is showing Card link with no copy in it. Probably just an authoring issue?

image

Otherwise, all positive changes. @jeffchew I'm okay with us fixing these as issues later. Whatever you think is best.

@jeffchew
Copy link
Member Author

jeffchew commented Sep 8, 2021

@shixiedesign let me do some quick fixes for these then can merge in, thank you @shixiedesign !

@jeffchew
Copy link
Member Author

jeffchew commented Sep 8, 2021

@shixiedesign for the content block media, it looks like it's actually rendering, so this might be an odd percy false positive I'll need to look into:

https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/7056/index.html?path=/story/components-content-block-media--default

@shixiedesign
Copy link
Contributor

With Locale modal's PR merged, this seems to be the last one to merge before we can review-approve the expressive sunset branch! Pretty optimistic we can merge it soon.

@jeffchew
Copy link
Member Author

jeffchew commented Sep 8, 2021

@shixiedesign I'm going to actually merge now and we can capture the feature section in a followup PR.

@jeffchew jeffchew added Ready to merge Label for the pull requests that are ready to merge and removed Needs design approval PRs on feature requests and new components have to get design approval before merge. 🛠️ fix needed labels Sep 8, 2021
@jeffchew jeffchew mentioned this pull request Sep 8, 2021
Closed
@kodiakhq kodiakhq bot merged commit 1ba3cd0 into carbon-design-system:feat/expressive-sunset Sep 8, 2021
@jeffchew jeffchew deleted the fix/card-link-react-expressive branch September 8, 2021 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IgnacioBecerra IgnacioBecerra IgnacioBecerra approved these changes

@ariellalgilmore ariellalgilmore ariellalgilmore approved these changes

@RichKummer RichKummer Awaiting requested review from RichKummer

Assignees

@jeffchew jeffchew

Labels
dev Needs some dev work Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
Sprint 21-18
Development

Successfully merging this pull request may close these issues.
6 participants
@jeffchew @ibmdotcom-bot @shixiedesign @oliviaflory @ariellalgilmore @IgnacioBecerra