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

[IM] API & UI for filtering Integrations by category #46089

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Sep 18, 2019

API & UI for filtering Integrations by category

Filters for "All" (not filtered) & each category. Only one active filter at a time.

closes #46070

45562-category-filters

I'll add some comments inline but see the individual commits for more context.

Did separate UI & API commits and tried to keep related changes together to make it easier to follow.

@jfsiii jfsiii requested review from ruflin, skh, jasonrhodes, hbharding and a team September 18, 2019 21:46
@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 18, 2019

@hbharding The active color used by EuiFacetButton doesn't work (too little contrast, IMO) with our background.

Screen Shot 2019-09-18 at 1 33 14 PM

How should we style active filters? Set the background color to the same as the container? Or use something else? Any other styling (outline, etc)?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Tested manually, works as expected.

ruflin added a commit to ruflin/package-registry that referenced this pull request Sep 19, 2019
Testing elastic/kibana#46089 I realised there is no package with only the metrics category. So here we go.
ruflin added a commit to ruflin/package-registry that referenced this pull request Sep 19, 2019
Testing elastic/kibana#46089 I realised there is no package with only the metrics category. So here we go.
ruflin added a commit to elastic/package-registry that referenced this pull request Sep 19, 2019
Testing elastic/kibana#46089 I realised there is no package with only the metrics category. So here we go.
@hbharding
Copy link
Contributor

@jfsiii good catch. I wasn't aware that the facet group used a background color for active. Lets use the same color as the container background here. euiColorLightestShade

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

Works great for existing categories 👍

When calling the API path directly for a category that doesn't exist, a 500 Internal Server Error occurs. I would have expected either a 200 with an empty list, or a 404.

We could do this now, or open an issue for making all API calls more robust. @jfsiii what do you think?

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 23, 2019

@skh good catch. 404 is what first comes to mind. I'll add in another commit shortly or open an issue.

@ruflin
Copy link
Member

ruflin commented Sep 23, 2019

@skh I assume this is for the internal API? For the registry API, I would also expect a 200 and an empty list: http://integrations-registry.app.elstc.co/search?category=foo The current response is null and not [], should this be fixed?

@skh
Copy link
Contributor

skh commented Sep 23, 2019

@ruflin Yes, this is for the internal API. The null returned from the registry might be what's triggering the error now, but I think Kibana-side code should be robust enough to handle it.

I have no opinion at all whether the two APIs need to be in sync with regard to return codes and error handling -- it's cleaner when they are, but I'm not sure it's worth the effort, as long as both behave robust and in a sensible way. So, 200 with [] or 404, both sounds good to me.

@ruflin
Copy link
Member

ruflin commented Sep 23, 2019

Followed up in the registry with elastic/package-registry#111

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 23, 2019

@ruflin Thanks. Approved elastic/package-registry#111

@skh This will flow through to the Manager's API, so no changes are required in the Manager code. Any other blockers to a 🚢 ?

@skh
Copy link
Contributor

skh commented Sep 23, 2019

This will flow through to the Manager's API, so no changes are required in the Manager code. Any other blockers to a 🚢 ?

Not from my side, looks good! 👍

@skh skh self-requested a review September 23, 2019 13:55
@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 23, 2019

@hbharding Looking into this, I found that the container color is euiPageBackgroundColor / #fafbfd and the facet button is already using euiColorLightestShade / #f5f7fa for the :focus style.

Looking at the Figma designs, I see that the background color for the screen is euiColorLightestShade which means the current pages aren't to spec. Fixed in fc3dfc2

@jfsiii jfsiii merged commit 6098d58 into elastic:feature-integrations-manager Sep 23, 2019
@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 23, 2019

@hbharding Some fallout from the changing the color of the containers

Screen Shot 2019-09-23 at 11 30 36 AM

The injected "Help us improve..." header background now doesn't match ours. Perhaps we can control this but it doesn't seem likely. Is this a sign that we should revert to using the prior color which, I believe is what other plugins use. Compare IM above with these other pages below:

Screen Shot 2019-09-23 at 11 51 52 AM

Screen Shot 2019-09-23 at 11 51 38 AM

Screen Shot 2019-09-23 at 11 51 02 AM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@hbharding
Copy link
Contributor

hbharding commented Sep 23, 2019

Good catch @jfsiii ! I'm still learning EUI, and today you taught me something :). I'll fix the figma files to use the proper page bg color, #FAFBFD

@jfsiii jfsiii deleted the 46070-list-view-category-filters branch December 30, 2019 16:13
@jen-huang jen-huang added the Team:Fleet Team label for Observability Data Collection Fleet team label Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants