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.

Sign up for GitHub

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

Jump to bottom

Migrate filter bar to React, EUI, and Typescript #25563

Merged
merged 104 commits into from
Jan 30, 2019
Merged

Migrate filter bar to React, EUI, and Typescript #25563

merged 104 commits into from
Jan 30, 2019

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Nov 12, 2018 •
edited
Loading

Summary

Rewrites the filter bar in React, EUI, and Typescript. Updates the look and feel of the filter bar and makes it consistent with the rest of K7.

image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@Bargs Bargs added Feature:Filters Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Nov 12, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@adriankirchner adriankirchner mentioned this pull request Nov 18, 2018
Closed
@lukasolson lukasolson force-pushed the euifyFilterBar branch 2 times, most recently from f1bdea0 to 3b158aa Compare December 4, 2018 19:07
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@timroes timroes mentioned this pull request Dec 11, 2018
Merged
@elasticmachine
Copy link
Contributor

💔 Build Failed

@Bargs Bargs requested a review from a team as a code owner December 19, 2018 17:03
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@snide snide mentioned this pull request Jan 2, 2019
Closed
8 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Copy link
Contributor Author

Bargs commented Jan 28, 2019

Hey @cchaos did you have a chance to look at those screenreader issues I mentioned?

@cchaos
Copy link
Contributor

cchaos commented Jan 28, 2019

Not yet, sorry, I will try to get to that next.

@cchaos
Copy link
Contributor

cchaos commented Jan 28, 2019

You need to add some title attributes. The below examples are how they're setup in the EUI docs:

Filter trigger button

const filterButtonTitle = `${this.state.filters.length} filters applied. Select to ${this.state.isFiltersVisible ? 'hide' : 'show'}.`;

    const filterTriggerButton = (
      <EuiFilterButton
        ...
        title={filterButtonTitle}
      >
        Filters
      </EuiFilterButton>
    );

EuiGlobalFilterItem

let title = `Filter: ${prefix} ${field}: "${value}". Select for more filter actions.`;
    if (isPinned) {
      title = `Pinned ${title}`;
    } else if (isDisabled) {
      title = `Disabled ${title}`;
    }

    const badge = (
      <EuiBadge
        ...
        title={title}
        iconOnClick={this.deleteFilter}
        iconOnClickAriaLabel={`Delete filter`}
        onClick={this.togglePopover}
        onClickAriaLabel="Filter actions"
      >
        {prefix}
        <span>{field}: </span>
        <span>&quot;{value}&quot;</span>
      </EuiBadge>
    );

ppisljar
ppisljar approved these changes Jan 29, 2019
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

tested it on windows with edge, seems to work fine (except that whole kibana performs super slow there and our sample dashboards are pretty much unusable)
tested also in IE11, works fine as well (performance is actually way better than in edge, there is some weird flickering happening when selecting dropdown values, but everything is usable.

LGTM

@Bargs Bargs dismissed Bamieh’s stale review January 30, 2019 00:49

Made requested updates

@Bargs
Copy link
Contributor Author

Bargs commented Jan 30, 2019

Thanks @cchaos, I added titles the badges. Could you take a look at the custom label switch as well? As far as I can tell we're passing in the exact same props as in the demo, but the label is not getting read out by the screenreader, even though it's working in the demo.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented Jan 30, 2019

It's actually not working in the EUI docs either because it requires an id to be passed so that it can create the necessary connection between the hidden checkbox and label via the htmlFor prop.

@timroes timroes mentioned this pull request Jan 30, 2019
Closed
5 tasks
@Bargs
add ID to EuiSwitch so that the label element can be correctly connected
54a515c 
to it and screenreaders will be able to read the text
@Bargs
Copy link
Contributor Author

Bargs commented Jan 30, 2019

@cchaos hmmm weird, you're right, adding an id fixed it in this PR. The global header demo doesn't pass an ID though and it was working for me in MacOS's VoiceOver. Strange.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs
Switch from react components to a function that returns simple display
303060c 
text so that it can be used in the filter apply modal as well
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit 410c094 into elastic:master Jan 30, 2019
Bargs added a commit to Bargs/kibana that referenced this pull request Jan 30, 2019
@Bargs
Revert "Migrate filter bar to React, EUI, and Typescript (elastic#25563…
4b9b3cf 
…)"

This reverts commit 410c094.
Bargs added a commit that referenced this pull request Jan 30, 2019
@Bargs
Revert "Migrate filter bar to React, EUI, and Typescript (#25563)" (#…
45b24af 
…29662)

This reverts commit 410c094.
Bargs added a commit to Bargs/kibana that referenced this pull request Jan 31, 2019
@Bargs
Revert "Revert "Migrate filter bar to React, EUI, and Typescript (ela…
a909d86 
…stic#25563)" (elastic#29662)"

This reverts commit 45b24af.
@Bargs Bargs mentioned this pull request Jan 31, 2019
Merged
Bargs added a commit that referenced this pull request Jan 31, 2019
@Bargs
Re-introduce new filter bar (#29752)
8a83955 
Fixes the build issues introduced in #25563 and re-introduces the new react/eui/typescript filter bar, essentially reverting the revert in #29662. I did have to resolve one merge conflict in query_bar.tsx, and re-deleted all of the old filter bar code where translation code had been added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasolson lukasolson lukasolson left review comments

@cjcenizal cjcenizal cjcenizal left review comments

@mistic mistic mistic left review comments

@stacey-gammon stacey-gammon stacey-gammon left review comments

@ppisljar ppisljar ppisljar approved these changes

@Bamieh Bamieh Bamieh left review comments

Assignees

@lukasolson lukasolson

@Bargs Bargs

Labels
Feature:Filters release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@Bargs @elasticmachine @cchaos @ppisljar @timroes @lukasolson @cjcenizal @mistic @Bamieh @stacey-gammon