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

[Discover] Deangularize the hits counter and create a react component #65631

Merged
merged 6 commits into from
May 8, 2020
Merged

[Discover] Deangularize the hits counter and create a react component #65631

merged 6 commits into from
May 8, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 7, 2020 •
edited
Loading

Summary

Fixes #65326

Deangularize the result counter (#65326). Created a hitsCounter React Component.

Checklist

@stratoula stratoula added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes labels May 7, 2020
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula self-assigned this May 7, 2020
@stratoula stratoula marked this pull request as ready for review May 7, 2020 13:03
@stratoula stratoula requested a review from a team May 7, 2020 13:03
@stratoula stratoula requested a review from a team as a code owner May 7, 2020 13:03
@stratoula stratoula requested review from kertal and removed request for kertal May 7, 2020 13:03
@kertal kertal self-requested a review May 7, 2020 14:19
cchaos
cchaos reviewed May 7, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Awesome. So happy to see more React conversions! My comments are mostly enhancements.

src/plugins/discover/public/application/components/hits_counter/hits_counter.tsx Outdated Show resolved Hide resolved
src/plugins/discover/public/application/components/hits_counter/hits_counter.tsx Show resolved Hide resolved
@stratoula stratoula added v7.9.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula stratoula changed the title Deangularize the hits counter and create a react component [Discover] Deangularize the hits counter and create a react component May 8, 2020
@stratoula stratoula requested review from cchaos, lizozom and kertal and removed request for lizozom May 8, 2020 09:09
kertal
kertal approved these changes May 8, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, tested locally (MacOs 10.14.6), Safari, Firefox, Chrome, all fine. Thx a lot for moving another brick of the Discover wall to React 🧱 !

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cchaos
Copy link
Contributor

cchaos commented May 8, 2020

UI looks great! Weirdly, I can't get the "Reset" button to work in Firefox or Chrome.

Steps:

  1. Pulled latest
  2. Launched new instance of ES
  3. Installed Ecommerce sample data
  4. Loaded [eCommerce] Orders saved search
  5. Removed some columns from the left sidebar
  6. Clicked "Reset" nothing happened then I'd get some errors in the console:

Screen Shot 2020-05-08 at 10 05 08 AM

@stratoula
Copy link
Contributor Author

stratoula commented May 8, 2020 •
edited
Loading

@cchaos we saw it and created an issue for that. Has nothing to do with this PR. The bug existed unfortunately. Here is the issue #65821

cchaos
cchaos approved these changes May 8, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 The primary goal of the PR, de-angularization, LGTM. Though the functionality is broken as mentioned in the comments.

@stratoula stratoula merged commit aea6d64 into elastic:master May 8, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request May 8, 2020
@stratoula @elasticmachine
[Discover] Deangularize the hits counter and create a react component (…
ffb8324 
…elastic#65631)

* Deangularize the hits counter and create a react component

* Add aria-label to button for accessibility

* Add icon to the link button and use EuiText

* Remove snapshots and test with findTestSubject

* Change toString with String()

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@stratoula stratoula mentioned this pull request May 8, 2020
Merged
stratoula added a commit that referenced this pull request May 11, 2020
@stratoula @elasticmachine
[Discover] Deangularize the hits counter and create a react component (…
7fd09b3 
…#65631) (#65881)

* Deangularize the hits counter and create a react component

* Add aria-label to button for accessibility

* Add icon to the link button and use EuiText

* Remove snapshots and test with findTestSubject

* Change toString with String()

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 11, 2020
@gmmorris
Merge branch 'master' into alerting/lazy-load-components
74f076f 
* master: (58 commits)
  [Drilldowns][chore] import dashboard url generator from plugin contract (elastic#64628)
  fix double flyouts in add panel flow (elastic#65861)
  Point 7.x to 7.9.0 in .backportrc.json
  Mount ui/new_platform applications in same div structure as Core (elastic#63930)
  [Uptime] Settings threshold validation (elastic#65454)
  Fix edit alert flyout to update initialAlert after edit (elastic#65359)
  Fix anomalies display on focused APM service map (elastic#65882)
  [SIEM][Detection Engine] Increases the template limit for ECS mappings
  [SIEM][CASE] Moves functional tests from "legacyEs" to "Es" (elastic#65851)
  [Metrics UI] Fix p95/p99 charts and alerting error (elastic#65579)
  [ML] Add job timing stats to anomaly jobs (elastic#65696)
  restore index pattern management data-test-subj's (elastic#64697)
  [Discover] Prevent whitespace wrapping of doc table header (elastic#52861)
  [SIEM] Fixes a CSS issue with Timeline field truncation (elastic#65789)
  Skipping failing tests. elastic#65867 elastic#65866 elastic#65865
  [Discover] Deangularize the hits counter and create a react component (elastic#65631)
  Tsvb less update (elastic#65467)
  [TSVB] Remove remaining lodash.set usage (elastic#65846)
  [Uptime] Add `a11y` tests (elastic#65514)
  [Uptime] Enable loading on monitor list (elastic#65670)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@kertal kertal kertal approved these changes

Assignees

@stratoula stratoula

Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discover] Deangularize result counter
5 participants
@stratoula @elasticmachine @kibanamachine @cchaos @kertal