-
Notifications
You must be signed in to change notification settings - Fork 78
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(block-group, block): add block-group component #11319
Conversation
packages/calcite-components/src/components/block-group/block-group.scss
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.
Awesome! Works great with mouse, keyboard, JAWS, and NVDA! 💪🏻
Some doc-related suggestions for consistency, otherwise lookin' great! 🏆
packages/calcite-components/src/components/block-group/block-group.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/block-group/block-group.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/block-group/block-group.tsx
Outdated
Show resolved
Hide resolved
…roup.tsx Co-authored-by: Kitty Hurley <[email protected]>
…roup.tsx Co-authored-by: Kitty Hurley <[email protected]>
…roup.tsx Co-authored-by: Kitty Hurley <[email protected]>
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.
Awesome! Mostly just some doc and naming nitpick / questions.
Only real feedback from design perspective would be thinking about some auto-collapsing when dragging open Blocks, but that could follow as further improvement (and, looking at these events, maybe be handled by a dev if they wanted to).
@@ -15,8 +15,6 @@ | |||
* @prop --calcite-loader-track-color: Specifies the component's track color. | |||
*/ | |||
|
|||
@import "../../assets/styles/animation"; |
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: These style import removals could be done in a separate PR.
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.
Agreed. Let's remove these separately. Applies to other non-block-related Sass files.
@property({ reflect: true }) disabled = false; | ||
|
||
/** When `true`, `calcite-block`s are sortable via a draggable button. */ | ||
@property({ reflect: true }) dragEnabled = false; |
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: the "via a draggable button" part of the doc could be cleaned up.... follow up - we use drag / sort in a lot of different places - is there any benefit to consolidating nomenclature there?
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.
Yeah that would be good to come up with a preferred way to phrase these. It depends on context, it could be a drag, a sort/reorder or a transfer. cc @geospatialem
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.
Aside from some comments, this LGTM!
Since this is deprecating some items, can you use the commit override to add a separate note per deprecation (example)?
packages/calcite-components/src/components/block-group/block-group.scss
Outdated
Show resolved
Hide resolved
export function updateBlockChildren(slotEl: HTMLSlotElement): void { | ||
const blockChildren = slotEl | ||
.assignedElements({ flatten: true }) | ||
.filter((el): el is Block["el"] => el?.matches(blockSelector)); |
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.
el
is guaranteed here, so ?
is not needed.
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.
Side note: we can start using Lit query decorators to simplify (e.g., https://lit.dev/docs/api/decorators/#queryAssignedElements).
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.
Do we have a follow up issue to start using queryAssignedElements? If not, can you create one? I think it would be nice to do this everywhere.
@@ -15,8 +15,6 @@ | |||
* @prop --calcite-loader-track-color: Specifies the component's track color. | |||
*/ | |||
|
|||
@import "../../assets/styles/animation"; |
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.
Agreed. Let's remove these separately. Applies to other non-block-related Sass files.
}); | ||
|
||
// Workaround for page.spyOnEvent() failing due to drag event payload being serialized and there being circular JSON structures from the payload elements. See: https://github.com/Esri/calcite-design-system/issues/7643 | ||
await page.$eval("#component2", (blockGroup: BlockGroup["el"]) => { |
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.
You might be able to use the new createEventTimePropValuesAsserter
for some of these assertions.
I'll create a refactor issue to wire it up in more tests.
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.
Can you create an issue to tackle it in both List and BlockGroup components?
packages/calcite-components/src/components/block-group/block-group.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/block-group/block-group.tsx
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/block-group/block-group.e2e.ts
Outdated
Show resolved
Hide resolved
We should give a heads up to teams about this.
I think that's fine since nested blocks aren't supported at the moment. |
Related Issue: #10382
Summary
calcite-block-group
calcite-block
elementscalcite-block
calcite-sort-handle
instead ofcalcite-handle
dragDisabled
sortHandleOpen
calcite-sort-handle
opening/closingcalciteBlockSortHandleBeforeClose
calciteBlockSortHandleBeforeOpen
calciteBlockSortHandleClose
calciteBlockSortHandleOpen
calcite-sort-handle
to work without asetSize
andsetPosition
for use withcalcite-sortable-list
.Notes:
calcite-sortable-list
withcalcite-block
will no longer just work. A user would need to set thecalcite-sortable-list
'shandleSelector
property to becalcite-sort-handle
instead ofcalcite-handle
.BlockGroup API Documentation
Properties
canPull
(detail: BlockDragDetail) => boolean
canPut
(detail: BlockDragDetail) => boolean
disabled
boolean
false
true
true
, interaction is prevented and the component is displayed with lower opacity.dragEnabled
boolean
false
true
true
,calcite-block
s are sortable via a draggable button.group
string
label
string
dragEnabled
istrue
and multiple block-group sorting is enabled withgroup
, specifies the component's name for dragging between block-groups.true
loading
boolean
false
true
true
, a busy indicator is displayed.Methods
setFocus
void