-
Notifications
You must be signed in to change notification settings - Fork 10
feat: new calcite-shell-center-row component and updated slot for calcite-shell #950
Conversation
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.
Looks good besides the changes to the .tsx
file.
… collapsed event.
… `collapsed` property.
… to put center-row after contextual-panel and added associated flex order.
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.
Looks good! just had a few comments/questions.
src/components/calcite-shell-center-row/calcite-shell-center-row.tsx
Outdated
Show resolved
Hide resolved
Oh! I forgot to add a Storybook. |
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.
Some stuff to fix! 🥳
src/components/calcite-shell-center-row/calcite-shell-center-row.e2e.ts
Outdated
Show resolved
Hide resolved
src/components/calcite-shell-center-row/calcite-shell-center-row.e2e.ts
Outdated
Show resolved
Hide resolved
src/components/calcite-shell-center-row/calcite-shell-center-row.e2e.ts
Outdated
Show resolved
Hide resolved
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 @driskull! |
I shall MURGLE unless you have any concerns, @jcfranco. :) |
Hurm...there might be a layout bug I overlooked. |
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.
Had a few comments, but this is looking noicely! 😎
expect(isVisible).toBe(true); | ||
}); | ||
|
||
it("should be accessible", async () => { |
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.
FIXME: Drop the curly braces to properly wire up the test. accessible
returns a promise that needs to be passed as the test (similar to the other tests using the helpers).
it("should be accessible", async () =>
accessible("<calcite-shell-center-row><div>content</div></calcite-shell-center-row>")
);
} | ||
])); | ||
|
||
it("should show row content", async () => { |
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.
What is this test for? It doesn't seem to be testing behavior.
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.
Heh...I was totes copying what shell-panel did. I can remove it.
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 @jcfranco ! I'll make updates. |
@jcfranco Updated! 🚀 |
Nice @asangma !!! 🥳 |
Thanks you rockers! 🤘🏽 |
There are some things that might need rethinking here, but I am not sure.
One super minor thing is whether we should rename bottom-panel to center-panel and give it an alignment prop for top and bottom.
The major things are
detached
with the bottom slot, right now, I'm just targeting that propertycc @kat10140 @jcfranco