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

[Emotion] Convert EuiSearchBar #7490

Merged
merged 4 commits into from
Feb 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/upcoming/7490.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
**CSS-in-JS conversions**

- Converted `EuiSearchBar` to Emotion
1 change: 0 additions & 1 deletion src/components/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@
@import 'markdown_editor/index';
@import 'tree_view/index';
@import 'side_nav/index';
@import 'search_bar/index';
@import 'selectable/index';
@import 'table/index';
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`SearchBar render - box 1`] = `
class="euiFlexGroup emotion-euiFlexGroup-responsive-wrap-m-flexStart-center-row"
>
<div
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1"
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1-euiSearchBar__searchHolder"
>
<div
class="euiFormControlLayout euiFormControlLayout--fullWidth"
Expand Down Expand Up @@ -45,7 +45,7 @@ exports[`SearchBar render - no config, no query 1`] = `
class="euiFlexGroup emotion-euiFlexGroup-responsive-wrap-m-flexStart-center-row"
>
<div
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1"
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1-euiSearchBar__searchHolder"
>
<div
class="euiFormControlLayout euiFormControlLayout--fullWidth"
Expand Down Expand Up @@ -84,7 +84,7 @@ exports[`SearchBar render - provided query, filters 1`] = `
class="euiFlexGroup emotion-euiFlexGroup-responsive-wrap-m-flexStart-center-row"
>
<div
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1"
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1-euiSearchBar__searchHolder"
>
<div
class="euiFormControlLayout euiFormControlLayout--fullWidth"
Expand Down Expand Up @@ -131,7 +131,7 @@ exports[`SearchBar render - provided query, filters 1`] = `
</div>
</div>
<div
class="euiFlexItem euiSearchBar__filtersHolder emotion-euiFlexItem-growZero"
class="euiFlexItem euiSearchBar__filtersHolder emotion-euiFlexItem-growZero-euiSearchBar__filtersHolder"
>
<div
class="euiFilterGroup emotion-euiFilterGroup-uncompressed"
Expand Down Expand Up @@ -194,7 +194,7 @@ exports[`SearchBar render - tools 1`] = `
</div>
</div>
<div
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1"
class="euiFlexItem euiSearchBar__searchHolder emotion-euiFlexItem-grow-1-euiSearchBar__searchHolder"
>
<div
class="euiFormControlLayout euiFormControlLayout--fullWidth"
Expand Down
1 change: 0 additions & 1 deletion src/components/search_bar/_index.scss

This file was deleted.

10 changes: 0 additions & 10 deletions src/components/search_bar/_search_bar.scss

This file was deleted.

33 changes: 33 additions & 0 deletions src/components/search_bar/search_bar.styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { css } from '@emotion/react';

import { UseEuiTheme } from '../../services';
import { euiBreakpoint, logicalCSS, mathWithUnits } from '../../global_styling';
import { euiFormVariables } from '../form/form.styles';

export const euiSearchBar__searchHolder = (euiThemeContext: UseEuiTheme) => {
const { maxWidth } = euiFormVariables(euiThemeContext);
Copy link
Member Author

@cee-chen cee-chen Jan 26, 2024

Choose a reason for hiding this comment

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

This very likely has performance implications as well (see #7486) and we likely need to memoize euiFormVariables (and other CSS-in-JS component style vars) going forward. I'll address that in a separate PR from this one

return css`
${logicalCSS(
JasonStoltz marked this conversation as resolved.
Show resolved Hide resolved
'min-width',
mathWithUnits(maxWidth, (x) => x / 2)
)}
`;
};

export const euiSearchBar__filtersHolder = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;
return css`
${euiBreakpoint(euiThemeContext, ['m', 'l', 'xl'])} {
/* Helps with flex-wrapping */
${logicalCSS('max-width', `calc(100% - ${euiTheme.size.base})`)}
}
`;
};
42 changes: 15 additions & 27 deletions src/components/search_bar/search_bar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
*/

import React, { useState } from 'react';
import { act } from '@testing-library/react';
import { mount } from 'enzyme';
import { fireEvent, act } from '@testing-library/react';

import { render } from '../../test/rtl';
import { requiredProps } from '../../test';
Expand Down Expand Up @@ -92,15 +91,15 @@ describe('SearchBar', () => {
test('calls onChange callback when the query is modified', () => {
const onChange = jest.fn();

const component = mount(
const { getByTestSubject } = render(
<EuiSearchBar
query="status:active"
onChange={onChange}
box={{ 'data-test-subj': 'searchbar' }}
/>
);

component.find('input[data-test-subj="searchbar"]').simulate('keyup', {
fireEvent.keyUp(getByTestSubject('searchbar'), {
key: keys.ENTER,
target: { value: 'status:inactive' },
});
Expand All @@ -114,7 +113,7 @@ describe('SearchBar', () => {

describe('hint', () => {
test('renders a hint below the search bar on focus', () => {
const component = mount(
const { getByTestSubject, queryByTestSubject } = render(
<EuiSearchBar
query="status:active"
box={{ 'data-test-subj': 'searchbar' }}
Expand All @@ -124,19 +123,14 @@ describe('SearchBar', () => {
/>
);

const getHint = () => component.find('[data-test-subj="myHint"]');

let hint = getHint();
expect(hint.length).toBe(0);
expect(queryByTestSubject('myHint')).toBeNull();

act(() => {
component.find('input[data-test-subj="searchbar"]').simulate('focus');
fireEvent.focus(getByTestSubject('searchbar'));
});
component.update();

hint = getHint();
expect(hint.length).toBe(1);
expect(hint.text()).toBe('Hello from hint');
expect(queryByTestSubject('myHint')).toBeInTheDocument();
expect(queryByTestSubject('myHint')).toHaveTextContent('Hello from hint');
});

test('control the visibility of the hint', () => {
Expand Down Expand Up @@ -164,28 +158,22 @@ describe('SearchBar', () => {
);
};

const component = mount(<TestComp />);
const getHint = () => component.find('[data-test-subj="myHint"]');
const { getByTestSubject, queryByTestSubject } = render(<TestComp />);

let hint = getHint();
expect(hint.length).toBe(0);
expect(queryByTestSubject('myHint')).toBeNull(); // Not visible on focus as it is controlled

act(() => {
component.find('input[data-test-subj="searchbar"]').simulate('focus');
fireEvent.focus(getByTestSubject('searchbar'));
});
component.update();

hint = getHint();
expect(hint.length).toBe(0); // Not visible on focus as it is controlled
expect(queryByTestSubject('myHint')).toBeNull(); // Not visible on focus as it is controlled

act(() => {
component.find('[data-test-subj="showHintBtn"]').simulate('click');
fireEvent.click(getByTestSubject('showHintBtn'));
});
component.update();

hint = getHint();
expect(hint.length).toBe(1);
expect(hint.text()).toBe('Hello from hint');
expect(queryByTestSubject('myHint')).toBeInTheDocument();
expect(queryByTestSubject('myHint')).toHaveTextContent('Hello from hint');
});
});
});
40 changes: 27 additions & 13 deletions src/components/search_bar/search_bar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import React, { Component, ReactElement } from 'react';

import { htmlIdGenerator } from '../../services/accessibility';
import { RenderWithEuiTheme, htmlIdGenerator } from '../../services';
import { isString } from '../../services/predicate';
import { EuiFlexGroup, EuiFlexItem } from '../flex';
import { EuiSearchBox } from './search_box';
Expand All @@ -18,6 +18,10 @@ import { CommonProps } from '../common';
import { EuiFieldSearchProps } from '../form/field_search';
import { EuiInputPopoverProps } from '../popover';

import {
euiSearchBar__searchHolder,
euiSearchBar__filtersHolder,
} from './search_bar.styles';
export { Query, AST as Ast } from './query';

export type QueryType = Query | string;
Expand Down Expand Up @@ -259,24 +263,20 @@ export class EuiSearchBar extends Component<EuiSearchBarProps, State> {

const toolsLeftEl = this.renderTools(toolsLeft);

const filtersBar = !filters ? undefined : (
<EuiFlexItem className="euiSearchBar__filtersHolder" grow={false}>
<EuiSearchBarFilters
filters={filters}
query={query}
onChange={this.onFiltersChange}
/>
</EuiFlexItem>
);

const toolsRightEl = this.renderTools(toolsRight);

const isHintVisible = hint?.popoverProps?.isOpen ?? isHintVisibleState;

return (
<RenderWithEuiTheme>
{(euiTheme) => (
<EuiFlexGroup gutterSize="m" alignItems="center" wrap>
{toolsLeftEl}
<EuiFlexItem className="euiSearchBar__searchHolder" grow={true}>
<EuiFlexItem
className="euiSearchBar__searchHolder"
css={euiSearchBar__searchHolder(euiTheme)}
grow={true}
>
<EuiSearchBox
{...box}
query={queryText}
Expand All @@ -298,9 +298,23 @@ export class EuiSearchBar extends Component<EuiSearchBarProps, State> {
}
/>
</EuiFlexItem>
{filtersBar}
{filters && (
Copy link
Member

@JasonStoltz JasonStoltz Feb 2, 2024

Choose a reason for hiding this comment

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

DELETED

Copy link
Member

Choose a reason for hiding this comment

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

While I was here, I also refactored some of the rendered JSX to not use a variable unnecessarily (which as react perf implications) but instead as an inline JSX conditional.

That's wild that this has a performance implication, I had no idea. I assume this is what you're referring to. I'm going to link to this change specifically as a reference on #7499

Copy link
Member Author

Choose a reason for hiding this comment

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

Kevin chatted about me with this in relation to #7374, but iirc the issue is that React creates the conditional node let as a new React component every single rerender. @kqualters-elastic feel free to chime in if I've misquoted you!

<EuiFlexItem
className="euiSearchBar__filtersHolder"
css={euiSearchBar__filtersHolder(euiTheme)}
grow={false}
>
<EuiSearchBarFilters
filters={filters}
query={query}
onChange={this.onFiltersChange}
/>
</EuiFlexItem>
)}
{toolsRightEl}
</EuiFlexGroup>
)}
</RenderWithEuiTheme>
);
}
}
Loading