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

feat(cta-section): web component version #4338

Merged

Conversation

IgnacioBecerra
Copy link
Contributor

@IgnacioBecerra IgnacioBecerra commented Oct 29, 2020

Related Ticket(s)

#3749
#3368

Description

Introducing the <dds-cta-section> component for Web Components, featuring Footer variations with Content Items and a Link List.

Default:
Screen Shot 2020-11-04 at 3 34 01 PM

Content Items Footer:
Screen Shot 2020-11-04 at 3 34 15 PM

Link List Footer:
Screen Shot 2020-11-04 at 3 34 27 PM

Changelog

New

  • <dds-cta-section> component
  • <dds-cta-section-item> component to wrap <dds-cta-section-heading> and <dds-cta-section-copy> with the correct styling

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Nice progress @IgnacioBecerra!

@@ -23,6 +23,8 @@ const { stablePrefix: ddsPrefix } = ddsSettings;
* The CTA Section pattern
*
* @element dds-cta-section
* @slot heading - The text heading.
* @slot buttons - The CTA Buttons.
*/
@customElement(`${ddsPrefix}-cta-section`)
class DDSCTASection extends StableSelectorMixin(LitElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems that this class should inherit DDSContentItem.

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 for the heads up! I'm wondering though, what are the main reasons it should inherit DDSContentItem?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for asking @IgnacioBecerra! One reason is that <dds-cta-section> seems to do a similar thing to <dds-content-item>, especially from the perspective of the API set. Another reason is that <CTASection> in React renders <ContentItem>, effectively extending <ContentItem> via React composition pattern, whereas React composition pattern doesn't work well with Web Components: https://github.com/carbon-design-system/carbon-for-ibm-dotcom/blob/00ad9c7/packages/web-components/docs/coding-conventions.md#preferring-class-inheritance-pattern-over-react-composition-pattern

}

.#{$dds-prefix}--content-block__copy {
display: inline-block;
font-size: 1.5rem;
font-size: calc(1.25rem + 0 * ((100vw - 20rem) / 62));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering where this change comes from...?

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 font size change comes from the React version, I think it was changed from 1.5rem to that calculation recently. Comes from the _cta-section.scss under the styles package.

@IgnacioBecerra IgnacioBecerra added the package: web components Work necessary for the IBM.com Library web components package label Nov 4, 2020
@IgnacioBecerra IgnacioBecerra marked this pull request as ready for review November 4, 2020 23:39
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Great progress - Thanks @IgnacioBecerra!

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Nov 4, 2020

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Nov 4, 2020

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thank you for the update @IgnacioBecerra!

@IgnacioBecerra
Copy link
Contributor Author

@asudoh Thanks for the suggestions! Now using tag selection for all the component styling. :)

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @IgnacioBecerra for all the updates!

Copy link
Member

@kennylam kennylam left a comment

Choose a reason for hiding this comment

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

@IgnacioBecerra Several components have had content removed (looks like markdown content) in the Percy snapshots. Any idea what's going on there?

@IgnacioBecerra
Copy link
Contributor Author

IgnacioBecerra commented Nov 6, 2020

@kennylam Yeah, I noticed that last night and was about comment about it. I'm using the new <dds-content-item and <dds-content-item-copy that @asudoh will be introducing in #4351. I didn't update the stories for all the components that use the old version of the components, thus the Percy snapshots.

Since this PR is somewhat dependent on #4351, should we wait until that one goes through before merging this one, or should I edit the stories to reflect the changes?

@jeffchew
Copy link
Member

jeffchew commented Nov 6, 2020

Screen Shot 2020-11-06 at 11 12 54 AM

Minor thing, looks like the formatting for the docs is slightly off, can you take a look?

@kennylam
Copy link
Member

kennylam commented Nov 6, 2020

Since this PR is somewhat dependent on #4351, should we wait until that one goes through before merging this one, or should I edit the stories to reflect the changes?

@IgnacioBecerra Yes good call. That one looks ready to merge soon. We can update here and make sure all is good.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thank you @IgnacioBecerra for the updates and rebasing!

```html
<dds-cta-section>
<dds-content-block-heading>Curabitur malesuada varius mi eu posuere</dds-content-block-heading>
<dds-cta-section-copy content="Lorem ipsum *dolor* sit amet"></dds-cta-section-copy>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<dds-cta-section-copy content="Lorem ipsum *dolor* sit amet"></dds-cta-section-copy>
<dds-cta-section-copy>Lorem ipsum *dolor* sit amet</dds-cta-section-copy>

## Props

<Props of="dds-cta-section" />

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Description markdown={markdownContents} />

@@ -0,0 +1,60 @@
import { Props, Description } from '@storybook/addon-docs/blocks';
import contributing from '../../../../../../docs/contributing-license.md';

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import markdownContents from '../../../../docs/markdown-contents.md';

<div class="bx--col-sm-4 bx-col-lg-12 bx--offset-lg-4">
<dds-cta-section>
<dds-content-block-heading>Curabitur malesuada varius mi eu posuere</dds-content-block-heading>
<dds-cta-section-copy content=${ifNonNull(copy)}></dds-cta-section-copy>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<dds-cta-section-copy content=${ifNonNull(copy)}></dds-cta-section-copy>
<dds-cta-section-copy>Want to discuss your options with a DevOps expert? Contact our sales team to evaluate your needs.</dds-cta-section-copy>

Comment on lines 72 to 74
<dds-cta-section-item-copy
>IBM DevOps partners have a wide range of expertise. Find one to build the right solution for you."
</dds-cta-section-item-copy>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<dds-cta-section-item-copy
>IBM DevOps partners have a wide range of expertise. Find one to build the right solution for you."
</dds-cta-section-item-copy>
<dds-cta-section-item-copy
>IBM DevOps partners have a wide range of expertise. Find one to build the right solution for you.</dds-cta-section-item-copy>

return html`
<dds-cta-section>
<dds-content-block-heading>${ifNonNull(heading)}</dds-content-block-heading>
<dds-cta-section-copy content=${ifNonNull(copy)}></dds-cta-section-copy>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<dds-cta-section-copy content=${ifNonNull(copy)}></dds-cta-section-copy>
<dds-cta-section-copy>ifNonNull(copy)</dds-cta-section-copy>

@IgnacioBecerra
Copy link
Contributor Author

@asudoh Thanks for catching those! Updated with new component usage.

@asudoh
Copy link
Contributor

asudoh commented Nov 7, 2020

Cool thanks @IgnacioBecerra for the update!

@jeffchew @kennylam Do you this is ready to go? Thanks!

Copy link
Member

@jeffchew jeffchew 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 added the Ready to merge Label for the pull requests that are ready to merge label Nov 9, 2020
@kodiakhq kodiakhq bot merged commit cfffb86 into carbon-design-system:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants