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 DocViewer table layout #43240

Merged
merged 83 commits into from
Aug 27, 2019

Conversation

kertal
Copy link
Member

@kertal kertal commented Aug 14, 2019

Summary

Replaces Angular of the Table Tab that's displayed when you unfold a list entry at Discover, at Discover's Context and Discover's Doc view.

Bildschirmfoto 2019-08-19 um 13 03 50

Bildschirmfoto 2019-08-19 um 13 04 14

Bildschirmfoto 2019-08-19 um 13 05 08

Part of #38646

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

aaronoah and others added 30 commits February 21, 2019 20:30
- to get rid of angular markup
- will be replaced with jest
@elastic elastic deleted a comment from elasticmachine Aug 26, 2019
@elastic elastic deleted a comment from elasticmachine Aug 26, 2019
@kertal kertal requested review from ryankeairns and cchaos August 26, 2019 06:17
@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, left some smallish comments but nothing really important. Tested in Chrome

import { flattenHitWrapper } from '../../../../data/public/index_patterns/index_patterns/flatten_hit';
import { DocViewTable } from './table';

// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not sure what's preferable, but you can achieve the same thing with } as unknown as IndexPattern

Copy link
Member Author

Choose a reason for hiding this comment

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

when I'm using unknown, i get a typescript complaint the next line:

indexPattern.flattenHit

doing it the right way, would mean I'd to add more than 40 props. I've chosen the pragmatic way, but I'm open for improvements :)


.kbnDocViewer__field {
width: 160px;
}

.kbnDocViewer__actionButton {
Copy link
Contributor

Choose a reason for hiding this comment

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

Really small thing, but the way the buttons are hidden is causing them to "stay" when you have clicked one and navigate away with the mouse which looks kinda strange:
Screenshot 2019-08-26 at 09 41 05

I filtered for present on continent name and then navigated away to the gender row, but the present filter stays focussed and thus visible. Initially I thought this happened on purpose and was some kind of state (if the filter is active, the button is shown all the time() and was confused it disappeared when doing something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, removed the opacity:1 of the elements focus state
FYI: @cchaos & @ryankeairns

/>
}
>
<i
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use IconTip here instead? https://elastic.github.io/eui/#/display/tooltip

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, another font awesome usage dependency removed:

Bildschirmfoto 2019-08-26 um 11 49 06

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

</React.Fragment>
`);
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @flash1293 Additionally to the changes you suggested, I've added Jest tests for the helper functions

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

👍 The new icons looks good! Thanks for removing the fontawesome icons :)

@kertal
Copy link
Member Author

kertal commented Aug 26, 2019

@cchaos are there more changes to be done? got still the _1 change requested_state. thx!

@kertal kertal merged commit b7bba21 into elastic:master Aug 27, 2019
kertal added a commit to kertal/kibana that referenced this pull request Aug 27, 2019
* Convert component to React

* EUI-ficate buttons, warnings and collapse button

* Add jest tests
kertal added a commit that referenced this pull request Aug 27, 2019
* Convert component to React

* EUI-ficate buttons, warnings and collapse button

* Add jest tests
@alexwizp alexwizp mentioned this pull request Oct 22, 2019
7 tasks
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 review Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants