[MarkdownEditorHelpButton] Add optional tooltipPosition#9546
[MarkdownEditorHelpButton] Add optional tooltipPosition#9546Zacqary wants to merge 3 commits intoelastic:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional tooltipPosition prop to EuiMarkdownEditorHelpButton so consumers (e.g., Dashboard’s markdown editor header layout) can control where the help tooltip renders without affecting existing usages (default remains top).
Changes:
- Extends
EuiMarkdownEditorHelpButtonprops with optionaltooltipPosition?: ToolTipPositions - Wires the new prop through to
EuiToolTip’spositionprop
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
weronikaolejniczak
left a comment
There was a problem hiding this comment.
Hey @Zacqary! Awesome to see you contribute to our lib, very happy about that! 💚
The ask is legitimate, the code is straightforward. Changelog is ok. Prop tables in docs will be automatically populated, no need to add docs about this.
The 3 concerns I have:
Prop name
Customarily in EUI we use e.g. <component>Props pattern, e.g. badgeProps, tooltipProps and spread them on the component. For tooltip specifically, we could omit content and children in the type and put content={syntaxTitle} after the spread.
Note: please use tooltipProps and not toolTipProps. It's "tooltip", one word.
Only works for standalone usage
I understand this will work for your use case where you render EuiMarkdownEditorHelpButton directly. But we need to consider the bigger picture.
If we do allow the position to be changed from the help button, we should likely allow it to be changed from EuiMarkdownEditor level as well (similarly as was done with toolbarProps before). It could look like:
footerProps?: {
helpButtonProps?: Pick<EuiMarkdownEditorHelpButtonProps, 'toolTipProps'>;
};
That would be the more consistent, complete API.
What for...?
Isn't a simpler solution just rendering that tooltip always to the bottom? That requires a design decision though.
Got it, I saw
By default, In our use case, we disable the footer and move the button to the header, which warrants the position change. I think it's unlikely anyone would want to change the tooltip position if they're putting the button in the footer. Happy to add this |
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
Summary
Adds an optional
tooltipPositionto the EuiMarkdownEditorHelpButton.This is needed for the Dashboard markdown editor, as we put the help button on the header and use a floating Edit/Preview toggle.
In elastic/kibana#253431 we're adding a Settings button, which we can successfully assign
position: bottom:But we can't do the same for the help button:
An undefined
tooltipPositiondefaults totop, so this change will have no impact on other uses of the help button.