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

De-angularize and EUI-ficate Discover Context control elements #44474

Merged

Conversation

kertal
Copy link
Member

@kertal kertal commented Aug 30, 2019

Summary

Convert Discover's Context control elements to React, introducing EUI elements

Bildschirmfoto 2019-09-04 um 12 35 45

Old Layout

Bildschirmfoto 2019-09-04 um 11 25 21

Bildschirmfoto 2019-09-04 um 11 25 40

New Layout

Bildschirmfoto 2019-09-04 um 11 24 24

Bildschirmfoto 2019-09-04 um 11 26 29

Note: I might adapt structure a bit when also the containing page is deangularized

Checklist

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

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

@kertal kertal self-assigned this Aug 30, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elastic elastic deleted a comment from elasticmachine Sep 4, 2019
@elastic elastic deleted a comment from elasticmachine Sep 4, 2019
@elastic elastic deleted a comment from elasticmachine Sep 4, 2019
@elastic elastic deleted a comment from elasticmachine Sep 4, 2019
@elastic elastic deleted a comment from elasticmachine Sep 4, 2019
@kertal kertal changed the title De-angularize discover context size picker De-angularize Discover Context control elements Sep 4, 2019
@kertal kertal marked this pull request as ready for review September 4, 2019 10:34
@kertal kertal requested a review from a team as a code owner September 4, 2019 10:34
@kertal kertal added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 4, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@kertal kertal added the v8.0.0 label Sep 4, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal
Copy link
Member Author

kertal commented Sep 7, 2019

there's another thing to be considered, every time you change the number, I request is triggered. but changing this would need a different design, like: you change the number, press return or another button is displayed to start the request

You could debounce the propagation of the input change, e.g. only fire a request if the user stopped typing for 500ms. But this is probably out of scope for this PR.

This should be done! But I think in another PR

no, let's debounce it, I've committed a suggestion. however, in the old version, the value was submitted, via RETURN or by leaving the field.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kertal
Copy link
Member Author

kertal commented Sep 9, 2019

The debouncing input approach would ease the problem of unwanted requests , but it would not solve it. this is especially a problem for the input at the bottom of the page, you start to type, pause a little, it just came to you mind that you don't need 10.000 records, to late, they were already requested. In the original version, the request was submitted on enter key or by leaving the input. I'd suggest to go with that pattern, and improve it a little by adding a button to submit the change (that's only displayed when the value was changed)

image

@flash1293
Copy link
Contributor

I would prefer the user experience of a debounced input field, but that's a subjective opinion. Also as these requests just do a simple search fetching documents (no costly aggregations), it feels like this should be fine.

@ryankeairns what do you think?

@kertal
Copy link
Member Author

kertal commented Sep 9, 2019

@flash1293 I do agree that these are light requests, I just think the 'I changed my mind' situation on the bottom bar can't happen with the submit button solution:

image

Also like using enter keys :). But I think both ways of solving this problem are practicable.

@kertal kertal changed the title De-angularize Discover Context control elements De-angularize and EUI-ficate Discover Context control elements Sep 9, 2019
@elastic elastic deleted a comment from elasticmachine Sep 9, 2019
@ryankeairns
Copy link
Contributor

ryankeairns commented Sep 9, 2019

In this situation, I feel the 'on submit' (press enter or leave the input) is the expected trigger.

Unlike a search ahead (autocomplete) where I would expect each keystroke to refine the search, this feels like an instance where I want to type a number with certainty prior to submitting. I don't feel the extra icon button is necessary, either.

- submit form onblur
- submit form on enter/return
- the increase of count is done by the react component
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor

Really like your solution, @kertal 👍 Great work!

Just one little thing that should be fixed - if you clear the input and submit it, you get hit with a bunch of errors:
Screenshot 2019-09-10 at 10 32 19

Should be a small fix.

@kertal
Copy link
Member Author

kertal commented Sep 10, 2019

Should be a small fix.

thx @flash1293! yes, I've added better checking for valid input between the range of MAX_CONTEXT_SIZE & MIN_CONTEXT_SIZE, and improved / deduplicated jest tests

@kertal kertal requested a review from flash1293 September 10, 2019 10:45
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, tested and works as expected. Great improvement! 👍

@kertal kertal merged commit 3798674 into elastic:master Sep 10, 2019
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 10, 2019
…-to-np-ready

* 'master' of github.com:elastic/kibana:
  Revert "Revert "[ci] compress jobs for CI stability" (elastic#44584)"
  add src/plugins to the list of plugin dirs to watch (elastic#45033)
  De-angularize and EUI-ficate Discover Context control elements (elastic#44474)
kertal added a commit to kertal/kibana that referenced this pull request Sep 10, 2019
…ic#44474)

* Create ActionBar component, merges loading button and size picker

* Use react component, remove angular code

* Migrate constants + state to typescript

* Remove unused increaseCount functions + tests

* Add jest test
kertal added a commit that referenced this pull request Sep 11, 2019
… (#45288)

* Create ActionBar component, merges loading button and size picker

* Use react component, remove angular code

* Migrate constants + state to typescript

* Remove unused increaseCount functions + tests

* Add jest test
@kertal kertal deleted the kertal-pr-convert-context-size-picker-to-react branch November 27, 2019 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants