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

[TextBasedEditor] Use resizable button from EUI #165935

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

stratoula
Copy link
Contributor

Summary

When I created the text based editor, the resizable button was not exported from EUI so I had to create my own custom solution. It was recently been exported and merged in kibana so now we can remove the custom solution and use the EUI component instead!

elastic/eui#6100

@stratoula stratoula added Feature:ES|QL ES|QL related features in Kibana Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.11.0 labels Sep 7, 2023
@stratoula stratoula marked this pull request as ready for review September 7, 2023 09:45
@stratoula stratoula requested review from a team as code owners September 7, 2023 09:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

Code review only

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, @stratoula! I left one small comment below.

Additionally, I was wondering if it would be possible to conditionally remove the bottom border on . uniSearchBar when the editor is in its expanded mode. That way we don't have the resizing icon overlaid on top of the dividing border when it's present (at least until we revisit the resizing styling as suggested in the Discover design refresh).

In any case, none of this is worth holding you up over, so I'm approving now. Thanks!

CleanShot 2023-09-07 at 16 04 02

Comment on lines 23 to 37
<div
css={css`
position: absolute;
bottom: 0;
left: 0;
right: 0;
`}
>
<EuiResizableButton
data-test-subj="TextBasedLangEditor-resize"
className="TextBasedLangEditor--resizableButton"
onMouseDown={onMouseDownResizeHandler}
onKeyDown={onKeyDownResizeHandler}
onClick={setFocus}
onTouchStart={onMouseDownResizeHandler}
/>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this wrapping div element necessary? Or can the CSS positioning be applied directly to the EuiResizableButton component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me test it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right, I removed it! 4e7a0bc

@stratoula
Copy link
Contributor Author

Additionally, I was wondering if it would be possible to conditionally remove the bottom border on . uniSearchBar when the editor is in its expanded mode.

It is possible but it will require some refactoring as this styling is outside of the editor component. We can explore in another PR, closer to the Discover redesign!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Sep 8, 2023

@stratoula stratoula merged commit a41b3ce into elastic:main Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants