Skip to content
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

Grafana/UI: Add new Splitter component #82357

Merged
merged 13 commits into from
Feb 14, 2024
Merged

Grafana/UI: Add new Splitter component #82357

merged 13 commits into from
Feb 14, 2024

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Feb 13, 2024

This brings a component @kaydelaney developed over from the scenes lib. As we need it in core directly (and not only through scenes SplitLayout).

  •  Refactors component a bit moving state logic out to a hook
  •  Removes unused props
  • Use our relatively new shared drag handle styles
  •  Adds storybook story
  •  Add dragPosition prop so we can have drag handles on the edge in panel edit (saves some space and we then use same UX pattern as with the drag handle in the drawer that is also on the edge)
  • Storybook docs
General-Layout-Splitter---Basic-.-Storybook.mp4

@torkelo torkelo requested review from a team as code owners February 13, 2024 09:52
@torkelo torkelo requested review from Ukochka, ashharrison90 and eledobleefe and removed request for a team February 13, 2024 09:52
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.4.x milestone Feb 13, 2024
@grafana-pr-automation grafana-pr-automation bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Feb 13, 2024
@grafana-pr-automation grafana-pr-automation bot requested a review from a team February 13, 2024 09:56
@grafana grafana deleted a comment from grafana-pr-automation bot Feb 13, 2024
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great and is really gonna help with our consistency 🚀

Add dragPosition prop so we can have drag handles on the edge in panel edit (saves some space and we then use same UX pattern as with the drag handle in the drawer that is also on the edge)

my potentially controversial UX opinion... i think we should always indent our drag handles and not have them on the edge. something like:

default onHover
explore image image
drawer image image

@torkelo
Copy link
Member Author

torkelo commented Feb 13, 2024

@ashharrison90 not sure, looks very strange with a drag handle floating inside the box. Feels much more intuitive with it the edge (and that way it does not conflict/overlap the content). We can tweak the drag handle styling maybe (in the future) to add some kind of grip bars to make it more noticeable or tweak the background.

Did check with UX (patrick) and agree that having on the edge is best, like we do for drawer and explore drawer. For panel edit the options pane there is no room to have it inside the box

@torkelo
Copy link
Member Author

torkelo commented Feb 13, 2024

@ashharrison90 here is having it inside the panel edit options pane, not great :)

Screenshot 2024-02-13 at 12 41 42

@ashharrison90
Copy link
Contributor

ashharrison90 commented Feb 13, 2024

@torkelo yeah in the panel edit case i'd still have it in the gap tbh. that feels like a slightly different use case to drawers anyway (resizing 2 panes vs. resizing one edge)

the main reason for me thinking that is that other drag handle examples are indented, e.g. slack on mobile:
tempFileForShare_20240213-123915

but that might be more of a mobile pattern 🤔
either way consistency is king so happy for anything as long it helps with that 😄

@torkelo
Copy link
Member Author

torkelo commented Feb 13, 2024

@ashharrison90 slack on desktop it's on the edge (without any drag handle)

@grafana grafana deleted a comment from grafana-pr-automation bot Feb 13, 2024
@torkelo
Copy link
Member Author

torkelo commented Feb 13, 2024

@ashharrison90 pushed storybook docs, and added some prop docs and simplified it a bit more. Think this is ready

Copy link
Contributor

@kaydelaney kaydelaney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@torkelo torkelo added add to changelog area/grafana/ui Issues that belong to components in the @grafana/ui library labels Feb 13, 2024
@torkelo torkelo removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Feb 13, 2024
@grafana grafana deleted a comment from grafana-pr-automation bot Feb 13, 2024
Comment on lines 19 to 20
verticalOffset = '0';
horizontalOffset = '0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
verticalOffset = '0';
horizontalOffset = '0';
verticalOffset = '0%';
horizontalOffset = '0%';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 43 to 57
kids,
containerRef,
firstPaneRef,
minDimProp,
splitterRef,
measurementProp,
handleSize,
onPointerUp,
onPointerDown,
onPointerMove,
onKeyDown,
onKeyUp,
onDoubleClick,
onBlur,
secondPaneRef,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a lot of returned "things". could we instead group some of them into one thing like splitterProps that can then be spread onto the splitter element? i'm mostly thinking about the interaction stuff (onPointer, onKey, onBlur, etc.), since they all have to go on the same element anyway

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also return the tabIndex, associated aria-value* attributes and role stuff as part of the splitterProps as well if you pass through the initialSize? that way you wouldn't have to manipulate the dom directly in the hook to change ariaValueNow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashharrison90 I moved most to a splitterProps and got rid of a few others, I did not like adding role, aria props tabIndex etc, because then I basically have move everything to the hook and there i no nothing in the markup generation left so the separation between callback / static logic and markup rendering becomes lost.

I agree if useSplitter was designed to be used outside this component is should probably return all props, but here it just a way to separate all the callbacks from the rendering

: never;
type HTMLElementOrNull = HTMLElement | null;

function measureElement<T extends HTMLElementOrNull>(ref: T): MeasureR<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can simplify our types a lot here if we restrict this to only allow an element to be passed (and not null). then our return type is always just

{ minWidth, maxWidth, minHeight, maxHeight }

and we just gate every use of measureElement behind an if statement, like

if (firstPaneRef.current) {
  const dim = measureElement(firstPaneRef.current);
  ...
}

this will also allow us to remove all(?) of the ! operators. maybe slightly more lines, but imo more readable and type-safe 😎

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion! updated, had to add a lot if statements though :)

@grafana-pr-automation grafana-pr-automation bot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Feb 13, 2024
@grafana grafana deleted a comment from grafana-pr-automation bot Feb 13, 2024
@torkelo torkelo removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Feb 14, 2024
@grafana grafana deleted a comment from grafana-pr-automation bot Feb 14, 2024
Copy link
Contributor

@ashharrison90 ashharrison90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove the type assertion - otherwise lgtm 👍

ref.style.height = savedHeight;
ref.style.flexGrow = savedFlex;

return { minWidth, maxWidth, minHeight, maxHeight } as MeasureResult;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove this type assertion 🥳

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, of course :)

@torkelo torkelo merged commit fe79541 into main Feb 14, 2024
18 of 19 checks passed
@torkelo torkelo deleted the splitter-component branch February 14, 2024 11:45
Ukochka pushed a commit that referenced this pull request Feb 14, 2024
* Initial thing working

* Update

* Progress

* Update

* Update

* Simplify a bit more

* minor refacto

* more review fixes

* Update

* review fix

* minor fix

* update
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/grafana/ui Issues that belong to components in the @grafana/ui library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants