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(callout): web-component created #3983

Merged
merged 17 commits into from
Sep 16, 2020
Merged

feat(callout): web-component created #3983

merged 17 commits into from
Sep 16, 2020

Conversation

raphaelamadeu-zz
Copy link
Contributor

Related Ticket(s)

#3638

Description

Callout internal pattern was created.

@raphaelamadeu-zz raphaelamadeu-zz added the package: web components Work necessary for the IBM.com Library web components package label Sep 16, 2020
@raphaelamadeu-zz raphaelamadeu-zz changed the title Feat/callout feat(callout): web-component created Sep 16, 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.

Nice progress @raphaelamadeu!

}

// eslint-disable-next-line class-methods-use-this
protected render() {
Copy link
Contributor

Choose a reason for hiding this comment

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

To align to the rest of the codebase:

Suggested change
protected render() {
render() {

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've updated it, thanks for the review!

Comment on lines 33 to 39
<div class="${prefix}--callout__container">
<div class="${prefix}--callout__column">
<div class="${prefix}--callout__content">
<slot />
</div>
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid duplicate elements (host and <div class="${prefix}--callout__container">):

Suggested change
<div class="${prefix}--callout__container">
<div class="${prefix}--callout__column">
<div class="${prefix}--callout__content">
<slot />
</div>
</div>
</div>
<div class="${prefix}--callout__column">
<div class="${prefix}--callout__content">
<slot />
</div>
</div>

Please make sure the host element has the style of .bx--callout__container. Please refer to https://github.com/carbon-design-system/ibm-dotcom-library/blob/v1.11.0-rc.0/packages/web-components/docs/coding-conventions.md#custom-element-itself-as-an-eleement for more details.

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've updated it, thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asudoh We still need the container, in order to maintain the structure seen in the styles package scss

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 @raphaelamadeu!

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2020

@jeffchew
Copy link
Member

@raphaelamadeu there are some build issues, can you take a look?

Screen Shot 2020-09-16 at 11 38 33 AM

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Sep 16, 2020

@jeffchew
Copy link
Member

@raphaelamadeu looks like there was an impact on the React side, can you doublecheck?

Screen Shot 2020-09-16 at 1 41 34 PM

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.

Marking as request changes as there looks like a number of diffs on the React side.

@raphaelamadeu-zz
Copy link
Contributor Author

Marking as request changes as there looks like a number of diffs on the React side.

@jeffchew They are now resolved.

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.

Thank you @raphaelamadeu !

@jeffchew jeffchew added the Ready to merge Label for the pull requests that are ready to merge label Sep 16, 2020
@kodiakhq kodiakhq bot merged commit aad9180 into carbon-design-system:master Sep 16, 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