-
Notifications
You must be signed in to change notification settings - Fork 159
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(logo-grid): web component created #4287
feat(logo-grid): web component created #4287
Conversation
Deploy preview created for package Built with commit: 3ccb565b0c73da1070afc1accd846ae06db6ef92 |
Deploy preview created for package Built with commit: 3ccb565b0c73da1070afc1accd846ae06db6ef92 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see you keep creating new components @raphaelamadeu!
@@ -83,7 +84,9 @@ | |||
} | |||
} | |||
|
|||
.#{$prefix}--logo-grid__container { | |||
.#{$prefix}--logo-grid__container, | |||
:host(#{$dds-prefix}-logo-grid):not(.#{$prefix}--logo-grid__no-border) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use CSS classes on our own Web Components, given it tends to clash with CSS classes application code sets:
:host(#{$dds-prefix}-logo-grid):not(.#{$prefix}--logo-grid__no-border) | |
:host(#{$dds-prefix}-logo-grid):not([hide-border]) |
@@ -0,0 +1,22 @@ | |||
{ | |||
"name": "ibmdotcom-web-components-quote-example", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name": "ibmdotcom-web-components-quote-example", | |
"name": "ibmdotcom-web-components-logo-grid-example", |
protected updated() { | ||
this.classList.toggle((this.constructor as typeof DDSLogoGrid).hideBorderClass, this.hideBorder); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use CSS classes on our own Web Components, given it tends to clash with CSS classes application code sets. We use attribute selectors instead:
protected updated() { | |
this.classList.toggle((this.constructor as typeof DDSLogoGrid).hideBorderClass, this.hideBorder); | |
} |
static get hideBorderClass() { | ||
return `${prefix}--logo-grid__no-border`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use CSS classes on our own Web Components, given it tends to clash with CSS classes application code sets. We use attribute selectors instead:
static get hideBorderClass() { | |
return `${prefix}--logo-grid__no-border`; | |
} |
class DDSLogoGrid extends StableSelectorMixin(DDSContentBlock) { | ||
static get stableSelector() { | ||
return `${ddsPrefix}--logo-grid`; | ||
} | ||
|
||
static get hideBorderClass() { | ||
return `${prefix}--logo-grid__no-border`; | ||
} | ||
|
||
@property({ attribute: 'hide-border', reflect: true, type: Boolean }) | ||
hideBorder = false; | ||
|
||
protected updated() { | ||
this.classList.toggle((this.constructor as typeof DDSLogoGrid).hideBorderClass, this.hideBorder); | ||
} | ||
|
||
// eslint-disable-next-line class-methods-use-this | ||
protected _renderContent() { | ||
return html` | ||
<div class="bx--content-block__children"> | ||
<div class="bx--logo-grid__row"> | ||
<slot></slot> | ||
</div> | ||
</div> | ||
`; | ||
} | ||
|
||
// eslint-disable-next-line class-methods-use-this | ||
protected _renderCopy() { | ||
return ''; | ||
} | ||
|
||
// `styles` here is a `CSSResult` generated by custom WebPack loader | ||
static get styles() { | ||
return css`${super.styles}${styles}`; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick for aligning with other classes in the codebase: Could you please re-group properties/methods, in the following order?
- Protected methods
- Public properties
- Static properties
There was a problem hiding this 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 @raphaelamadeu!
protected updated() { | ||
this.classList.toggle((this.constructor as typeof DDSLogoGrid).hideBorderClass, this.hideBorder); | ||
this.toggleAttribute('hide-border', this.hideBorder); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@property({ attribute: 'hide-border', reflect: true, type: Boolean })
takes care of this automatically.
protected updated() { | |
this.classList.toggle((this.constructor as typeof DDSLogoGrid).hideBorderClass, this.hideBorder); | |
this.toggleAttribute('hide-border', this.hideBorder); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @raphaelamadeu for the updates!
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
import { customElement, html, property } from 'lit-element'; | ||
// import settings from 'carbon-components/es/globals/js/settings'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to remove this line?
import DDSImage from '../image/image'; | ||
import StableSelectorMixin from '../../globals/mixins/stable-selector'; | ||
|
||
// const { prefix } = settings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to remove this line?
protected updated() { | ||
this.toggleAttribute('hide-border', this.hideBorder); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@property({ attribute: 'hide-border', reflect: true, type: Boolean })
takes care of this automatically.
protected updated() { | |
this.toggleAttribute('hide-border', this.hideBorder); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raphaelamadeu Have you tried to removing this method? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 - Thanks @raphaelamadeu for all the updates!
@raphaelamadeu for the documentation, can you provide some more details of what the |
@raphaelamadeu can you add stable selectors? |
This reverts commit 4620b58.
Related Ticket(s)
#3780
Description
Web component version created after the react component.