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

[discover] De-angularize index pattern selection #46347

Merged

Conversation

kertal
Copy link
Member

@kertal kertal commented Sep 23, 2019

Summary

Convert Discover's index pattern selection to EUI and adapt a new design.

Bildschirmfoto 2019-09-25 um 17 49 26

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 changed the title Kertal pr 2019 09 23 discover index pattern select De-angularize discover index pattern select Sep 23, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal changed the title De-angularize discover index pattern select De-angularize discover index pattern selection Sep 24, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal kertal self-assigned this Sep 26, 2019
@kertal kertal changed the title De-angularize discover index pattern selection [discover] De-angularize discover index pattern selection Sep 26, 2019
@kertal kertal changed the title [discover] De-angularize discover index pattern selection [discover] De-angularize index pattern selection Sep 26, 2019
@kertal kertal marked this pull request as ready for review September 27, 2019 08:25
@kertal kertal requested a review from a team as a code owner September 27, 2019 08:25
@kertal kertal requested review from a team and flash1293 September 27, 2019 08:29
@kertal kertal added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Sep 27, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@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.

Looking much better. There were some little differences between this and Lens, so I've opened a design PR against this PR.

You can view/merger it here:
👉 PR → kertal#2

I realize there is more design work to be done here in the future, but the new selector heading was very closely matched to the headings below (Available, Selected). This makes some visual changes to those subheadings.

The height of the index pattern title did not match the heigh of the ComoBox which resulted in a jump, which I have fixed here. Also, the ComboBox dropdown was wider than its parent input, so I've added padding to the entire sidebar and removed it from the child elements.

Let me know if you have questions!

…index-pattern-select

Discover sidebar style updates
…github.com:kertal/kibana into kertal-pr-2019-09-23-discover-index-pattern-select
@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2019

@ryankeairns I actually have a PR that is soon to merge for Lens that changes this behavior to not swap the text for a combo box but instead uses the EuiSelectable in a popover. If you'd like to align these utilities, you can consider the new pattern in this PR #46350 or we can collaborate after 7.5.

@kertal
Copy link
Member Author

kertal commented Sep 27, 2019

@ryankeairns I actually have a PR that is soon to merge for Lens that changes this behavior to not swap the text for a combo box but instead uses the EuiSelectable in a popover. If you'd like to align these utilities, you can consider the new pattern in this PR #46350 or we can collaborate after 7.5.

I'm aware of this changes, think we should finish both PRs separately, and then create a shared component for both, shouldn't be much effort

@kertal kertal requested a review from ryankeairns September 27, 2019 15:01
@cchaos
Copy link
Contributor

cchaos commented Sep 27, 2019

create a shared component for both

Agreed! I already broke the popover itself into its own component.

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.

LGTM. As discussed, we will incorporate the newer Lens-style design (using a popover instead of Combobox) in a follow-up PR.

@kertal
Copy link
Member Author

kertal commented Sep 27, 2019

@ryankeairns thx for the changes btw. 🙏

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kertal
Copy link
Member Author

kertal commented Sep 30, 2019

@elasticmachine merge upstream

@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.

Tested and works, LGTM 👍

Doesn't have to be solved in this PR, but we should definitely give the side bar a min width when migrating it, in some cases there is really little space for the index pattern title:
Screenshot 2019-09-30 at 09 50 17

border: none;
border-radius: 0;
}
.sidebar-item-title,
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but we should move this css to our regular class name style at some point.

);
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@kertal
Copy link
Member Author

kertal commented Sep 30, 2019

Tested and works, LGTM 👍

Doesn't have to be solved in this PR, but we should definitely give the side bar a min width when migrating it, in some cases there is really little space for the index pattern title:
Screenshot 2019-09-30 at 09 50 17

Thx you for the review and valuable hints dear @flash1293 ⚡️ . about the min width, I've already experimented with making the width customizable via drag and drop. this will be the direction to go, and then you should also have a min width. currently this part is still bootstrap/angular, but that will change soon

@kertal kertal merged commit f9cc456 into elastic:master Sep 30, 2019
kertal added a commit to kertal/kibana that referenced this pull request Sep 30, 2019
* Implementation of DiscoverIndexPattern React component

* Implement usage of the DiscoverIndexPattern

* Adapt tests
kertal added a commit that referenced this pull request Oct 2, 2019
* Implementation of DiscoverIndexPattern React component

* Implement usage of the DiscoverIndexPattern

* Adapt tests
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:enhancement 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.

5 participants