feat(cxl-ui): refactor light-card #346
Conversation
size-limit report 📦
|
HenerHoop
force-pushed
the
hener/feat/refactor-light-card
branch
3 times, most recently
from
d3d0e55
to
6330383
Compare
HenerHoop
force-pushed
the
hener/feat/refactor-light-card
branch
from
6330383
to
fa4439d
Compare
HenerHoop
force-pushed
the
hener/feat/refactor-light-card
branch
from
fa4439d
to
c41ac7d
Compare
|
|
||
| @property({ type: Boolean, reflect: true }) new = false; | ||
|
|
||
| @property({ type: Boolean, reflect: true }) portrait = false; |
There was a problem hiding this comment.
I think this should be defined by theme. @freudFlintstone @anoblet WDYT?
There was a problem hiding this comment.
It's only used in CSS, so yes, theme would be more consistent with the rest of the components. It is better to have a property only if the component needs to conditionally, and maybe dynamically, render parts of the template depending on that property.
|
|
||
| @property({ type: Boolean, reflect: true }) completed = false; | ||
|
|
||
| @property({ type: String, attribute: 'link' }) link = ''; |
There was a problem hiding this comment.
Is it named link in other cards not url?
| _renderTime() { | ||
| if ( this.time ) { | ||
| return html` | ||
| <cxl-time time=${this.time} ?show-icon=${this.showTimeIcon}></cxl-time> | ||
| `; |
There was a problem hiding this comment.
As far as I see we don't have time anywhere in the design. @heshfekry can you confirm?
There was a problem hiding this comment.
There are no component states with time in the figma file, or in the dashboard, apparently.
https://www.figma.com/file/RTj2PtUSrCOUFtljOiq0W3/CXL-2.0?type=design&node-id=2728-497829&mode=dev
HenerHoop
force-pushed
the
hener/feat/refactor-light-card
branch
from
c41ac7d
to
6345e18
Compare
HenerHoop
force-pushed
the
hener/feat/refactor-light-card
branch
from
6345e18
to
bbebec0
Compare
|
|
||
| @customElement('cxl-light-card') | ||
| export class CXLLightCardElement extends CXLBaseCardElement { | ||
| export class CXLLightCardElement extends LitElement { |
There was a problem hiding this comment.
We reverted course on this change in the last call with hesh, remember? We decided to edit the current cxl-light-card, which extends CXLBaseCardElement, instead of doing it from scratch. It looks like you copied a lot of code from other files. That's usually a sign we are going in the wrong direction.
There was a problem hiding this comment.
@HenerHoop you missed this, and now it looks like you copied a lot of what base-card already handles. This is becoming way too complicated for a component that doesn't even have logic. We got lost here.
There was a problem hiding this comment.
I'm sorry, @HenerHoop, looks like I forgot to submit the old review. I take responsibility for that. But you did do exactly the opposite of what was agreed in a call with Hesh, so that's still a problem. We need this to go forward, and it looks to be working well now, so let's push it over the line and add a backlog task to fix it later.
|
|
||
| @property({ type: Boolean, reflect: true }) new = false; | ||
|
|
||
| @property({ type: Boolean, reflect: true }) portrait = false; |
There was a problem hiding this comment.
It's only used in CSS, so yes, theme would be more consistent with the rest of the components. It is better to have a property only if the component needs to conditionally, and maybe dynamically, render parts of the template depending on that property.
| _renderTime() { | ||
| if ( this.time ) { | ||
| return html` | ||
| <cxl-time time=${this.time} ?show-icon=${this.showTimeIcon}></cxl-time> | ||
| `; |
There was a problem hiding this comment.
There are no component states with time in the figma file, or in the dashboard, apparently.
https://www.figma.com/file/RTj2PtUSrCOUFtljOiq0W3/CXL-2.0?type=design&node-id=2728-497829&mode=dev
|
Closing in favor of #353 |
https://app.clickup.com/t/86ayhhg16