ElementSelection: New element selection context to support selecting elements #97029
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…lements like panels
torkelo
requested review from
aocenas,
ashharrison90 and
Ukochka
and removed request for
a team
torkelo
changed the title
torkelo
changed the title
torkelo
commented
Nov 26, 2024
Comment on lines
89
to
114
| <Stack gap={3} alignItems={'center'}> | ||
| <Checkbox | ||
| label="Enable element selection" | ||
| value={elementSelectionState.enabled} | ||
| onChange={() => onToggleElementSelection()} | ||
| /> | ||
| Selected: | ||
| {elementSelectionState.selected.map((item) => ( | ||
| <span key={item.id}>{item.id}</span> | ||
| ))} | ||
| </Stack> | ||
| <ElementSelectionContext.Provider value={elementSelectionState}> | ||
| <VizGridLayout> | ||
| <PanelChrome title={'Panel title 1'} selectionId="panel-1"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 2'} selectionId="panel-2"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 3'} selectionId="panel-3"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 4'} selectionId="panel-4"> | ||
| Hello | ||
| </PanelChrome> | ||
| </VizGridLayout> |
There was a problem hiding this comment.
Regarding this demo impl, thoughts
- Remove it
- Move it to a storybook story for ElementSelectionContext
- Keep it here
There was a problem hiding this comment.
Keeping it here feels weird. I'd move it to storybook or remove it.
bfmatei
approved these changes
Nov 27, 2024
There was a problem hiding this comment.
I really like the overall implementation of this. Left two comments in the code but overall I really like it.
A couple of additional comments:
- let's avoid having the timer for selection clearing. I think we can improve the existing solution in the future if it becomes a problem
- I find providers pretty awful to work with, especially because they're causing more nesting in JSX which can lead to ugly code. However, one of my comments was about having a hook that consumes the context and encapsulates the entire logic of handling selection. Or we can have both and allow devs to use whatever feels better for them?
| } | ||
|
|
||
| const isSelected = context.selected.some((item) => item.id === id); | ||
| const onSelect = useCallback<React.PointerEventHandler>( |
There was a problem hiding this comment.
I think this is breaking the rules of hooks again.
There was a problem hiding this comment.
not really, if theres is no context and a context is added there will be an unmount/remount
so an early exit when there is no context is fine
Comment on lines
89
to
114
| <Stack gap={3} alignItems={'center'}> | ||
| <Checkbox | ||
| label="Enable element selection" | ||
| value={elementSelectionState.enabled} | ||
| onChange={() => onToggleElementSelection()} | ||
| /> | ||
| Selected: | ||
| {elementSelectionState.selected.map((item) => ( | ||
| <span key={item.id}>{item.id}</span> | ||
| ))} | ||
| </Stack> | ||
| <ElementSelectionContext.Provider value={elementSelectionState}> | ||
| <VizGridLayout> | ||
| <PanelChrome title={'Panel title 1'} selectionId="panel-1"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 2'} selectionId="panel-2"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 3'} selectionId="panel-3"> | ||
| Hello | ||
| </PanelChrome> | ||
| <PanelChrome title={'Panel title 4'} selectionId="panel-4"> | ||
| Hello | ||
| </PanelChrome> | ||
| </VizGridLayout> |
There was a problem hiding this comment.
Keeping it here feels weird. I'd move it to storybook or remove it.
| @@ -14,26 +26,93 @@ export const TestStuffPage = () => { | |||
| }; | |||
|
|
|||
| const notifyApp = useAppNotification(); | |||
| const [elementSelectionState, setElementSelectionState] = useState<ElementSelectionContextState>({ | |||
There was a problem hiding this comment.
I wonder if we should make this a standalone hook that can be reused.
There was a problem hiding this comment.
@bfmatei maybe, not sure what the real dashboard implementation will look like yet. But after we make that one we could we can see how re-usable it is
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A quick proof of concept of an element selection solution.
We are exploring an edit pane in dashboards that shows actions & options for any element or elements (multiple) selected. See first POC here: #96895
In that POC, I used a very hacky global onClick handler and element attributes to mark elements as selectable and selected. However, because it relied on DOM element manipulation to add the selected state class, the selection state is visually lost whenever react re-renders. This PR is exploring a more robust react approach to element selection via a context.
Context has
Point of feedback
Outside the scope of this PR (So ignore)
element_selection_context.mp4