feat(shell-center-row): add slot for action-bar #1015
Conversation
|
@kat10140 Let me know if you want to sync re: doc for this. |
|
@driskull Just realized I need to add some e2es and stuff. |
|
Hokay! Added E2E for action-bar slot and tweaked Readme. |
| @@ -343,7 +343,7 @@ <h2>Boomer v Millennial - California</h2> | |||
| <tr> | |||
| <td class="sml"><calcite-icon icon="square" scale="s"></calcite-icon></td> | |||
| <td>01</td> | |||
| <td>Can't keep Johnny down.</td> | |||
| <td>LAST</td> | |||
There was a problem hiding this comment.
Nitpick: if this is meant to call out being last, can you update this to "This should be last" or something similar?
| border-left: none; | ||
| border-right: 1px solid var(--calcite-app-border); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could you run Prettier on this file? It should add a trailing newline and ensure space between props and values.
| border-right: 1px solid var(--calcite-app-border); | ||
| } | ||
| ::slotted(calcite-action-bar[position="end"]) { | ||
| border-left: 1px solid var(--calcite-app-border); |
There was a problem hiding this comment.
Can you store 1px solid var(--calcite-app-border) in a var and reuse?
|
|
||
| expect(actionBarContainer).not.toBeNull(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This needs a test for the start/end positioning of action-bar.
| ); | ||
|
|
||
| const actionBar = getSlotted<HTMLCalciteActionBarElement>(el, SLOTS.actionBar); |
There was a problem hiding this comment.
suggestion:
const actionBarNode = getSlotted<HTMLCalciteActionBarElement>(el, SLOTS.actionBar) ? (
<div class={{ [CSS.actionBarContainer]: true, [CSS_UTILITY.rtl]: rtl }}>
<slot name={SLOTS.actionBar} />
</div>
) : null;| </div> | ||
| ); | ||
|
|
||
| const children = [contentNode]; |
There was a problem hiding this comment.
suggestion:
const children = [actionBarNode, contentNode];
if(actionBar.position === "end"){
children.reverse();
}There was a problem hiding this comment.
Wouldn't I need to nest this in a conditional for whether or not there's an action-bar?
There was a problem hiding this comment.
Oh...I see. Lemme try dat.
There was a problem hiding this comment.
I don't think so. it won't render anything null
|
|
||
| const actionBar = getSlotted<HTMLCalciteActionBarElement>(el, SLOTS.actionBar); | ||
|
|
||
| if (!actionBar) { |
There was a problem hiding this comment.
suggestion: remove this block.
|
Thanks @jcfranco. I'll do all the things. |
…or action-bar position. Fixed typo in SCSS. (#1007)
| @@ -73,7 +73,7 @@ export class CalciteActionBar { | |||
| /** | |||
| * Arranges the component depending on the elements 'dir' property. | |||
| */ | |||
| @Prop({ reflect: true }) position: CalcitePosition; | |||
| @Prop({ reflect: true }) position: CalcitePosition = "start"; | |||
There was a problem hiding this comment.
Do we need this to have a value or can we assume start if not present?
There was a problem hiding this comment.
I am using that attribute in the styles in center-row.
There was a problem hiding this comment.
Although...yeah. I see. I'll update.
There was a problem hiding this comment.
@driskull Done. Thanks.
| @@ -10,6 +10,8 @@ | |||
| --calcite-app-shell-center-row-height-small: 33%; | |||
| --calcite-app-shell-center-row-height-medium: 70%; | |||
| --calcite-app-shell-center-row-height-large: 100%; | |||
|
|
|||
| --calcite-app-shell-center-row-border: 1px solid var(--calcite-app-border); | |||
There was a problem hiding this comment.
Sorry, I meant to suggest a sass var for this.
There was a problem hiding this comment.
Oh. Is it better as a sass var?
There was a problem hiding this comment.
@jcfranco I'm down to do that but we would wanna update all the sass files to use that var, yeah?
Related Issue: #1007
Summary
Add slot for ActionBar.