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

feat: Provide scaler search and list external scalers from Artifact Hub #805

Merged
merged 19 commits into from
Aug 2, 2022

Conversation

thisisobate
Copy link
Contributor

@thisisobate thisisobate commented Jul 7, 2022

Provide a description of what has been changed
After analyzing the issue, I realized it's best to avoid creating an entirely new page for external scalers. Both built-in and external scalers fall under the same scaler category and thus, I think it's best to have both scaler types present on one landing page. By so doing, we maintain a clean information architecture that the user loves.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Create tabs for switching between built-in and external scalers
  • Embed widget to show external scaler hosted in artifacthub
  • Add search input
  • Style search input
  • Create search index using lunr
  • Implement search functionality
  • Hide search result count when viewed in external scalers
  • Fix search result link for built in scalers
  • Show total scaler count by default
  • Filter by input change instead of submit
  • revert title to page name instead
  • Disable external scaler button when built in scaler is searched
  • Add a section for wildcard search in faq
  • clean up code

Fixes #577
Relates to #412

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@netlify
Copy link

netlify bot commented Jul 7, 2022

Deploy Preview for keda ready!

Name Link
🔨 Latest commit 03cd304
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/62e932c460b71000098dc3e8
😎 Deploy Preview https://deploy-preview-805--keda.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@thisisobate
Copy link
Contributor Author

@chalin @nate-double-u
Any thoughts?

@nate-double-u
Copy link
Contributor

nate-double-u commented Jul 8, 2022

I like that you're splitting them up like that, the external page looks especially clean. Let's discuss if there is a way to break up the built-in set into categories, this may shorten the built in page, and make things a bit easier to find.

@tomkerkhove
Copy link
Member

@kedacore/keda-maintainers Thoughts on this PR?

My current thinking is that I love it, but believe the initial request was to have a separate page for external scalers. In theory having them in the same with a filter is OK; but given their layout is different I'm doubting if we should go with that separate page.

With regards to the built-in scalers, though, we were recently discussing the way we display them on Scalers page vs our landing page and believe that the current Scalers page is more ideal given the amount of scalers that we have which makes it easier to see all of them at a glance without scrollign

@JorTurFer
Copy link
Member

I ❤️ how it looks, the only feedback I have it's: Could we make the scaler cards a bit smaller to introduce 3 columns per row? Now The scroll is huge. makes sense?

@thisisobate
Copy link
Contributor Author

thisisobate commented Jul 8, 2022

@nate-double-u Breaking up built-in scalers into categories is a great idea but I think it's best we make it a last resort. Tbh, we can explore other easier approaches like @JorTurFer's suggestion

@thisisobate
Copy link
Contributor Author

@tomkerkhove @JorTurFer I agree with you. The scroll will become too long as we add more scalers.
Would try the 3-columns approach instead

@thisisobate
Copy link
Contributor Author

thisisobate commented Jul 11, 2022

I played around with the 3-columns approach but still, it was quite long. I'm considering two approaches here: filter-by-selection, and filter-by-search.

  1. filter by selection: this would involve sorting the contents by categories. I'm considering using the maintainer keyword as a category. This works perfectly if we're dealing with a small number of categories.

  2. filter by search: this would involve using a search input for filtering of scalers. This works perfectly when you're dealing with a huge number of categories and accommodates an ever-growing list.

My suggestion: I think the first option suits the needs of this project.

@tomkerkhove @JorTurFer Any thoughts?

@JorTurFer
Copy link
Member

What do you propose as a category filter? I mean, maintainer it's not a useful search value. I think that a search filter by name it's a good option. WDYT?

@thisisobate
Copy link
Contributor Author

thisisobate commented Jul 11, 2022

@JorTurFer The category filter I'm talking about is something like a list with checkboxes showing all the scalers maintained by a specific organization. When a user clicks on one of the categories (e.g Microsoft), it shows all the scalers maintained by Microsoft.

A search filter by name isn't bad either. I think it would work perfectly.

@thisisobate
Copy link
Contributor Author

@tomkerkhove Any thoughts?

@tomkerkhove
Copy link
Member

Filtering by category is being tracked in #412 so I would not try to squeeze this in this PR unless we want to do everything in my opinion.

I think having a search box would be nice a good addition as well as a "number of scalers" showing what matches the search value (or show total available scalers when nothing is searched for)

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@tomkerkhove
Copy link
Member

Thanks for adding the search! However the box was a bit hard to find and didn't realize I had to press enter 😅

@thisisobate
Copy link
Contributor Author

thisisobate commented Jul 15, 2022

Lol...I know
Added styling and UX fixes to my todo list in the PR description above

Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
Signed-off-by: thisisobate <obasiuche62@gmail.com>
@v-shenoy
Copy link
Contributor

This is probably in the works, but typing a word partially doesn't bring up the scaler.

For example, when searching for the ActiveMQ scaler, If I start typing "active" it won't show until the text "activemq" is fully present.

@thisisobate
Copy link
Contributor Author

The search actually supports wildcard search. All you need to do is append asterisks (e.g "active*") at the end of the search query and it returns all the appropriate matches.

You can read more here

@v-shenoy

@v-shenoy
Copy link
Contributor

Ah, that makes sense. Just one thing though, as a user the natural assumption (at least I think it to be) is that search is fuzzy.

So, maybe just a line mentioning the usage and linking to this doc could be added?

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@thisisobate thisisobate requested a review from tomkerkhove August 1, 2022 07:25
@tomkerkhove
Copy link
Member

Is there also a way to reduce the spacing here? (not new issue though, so separate PR is ok)

image

@thisisobate
Copy link
Contributor Author

thisisobate commented Aug 1, 2022

Is there also a way to reduce the spacing here? (not new issue though, so separate PR is ok)

Yes...it's ok to have this as a separate PR @tomkerkhove

Signed-off-by: thisisobate <obasiuche62@gmail.com>
@tomkerkhove tomkerkhove requested a review from JorTurFer August 1, 2022 11:41
@tomkerkhove tomkerkhove merged commit 4f36f7f into kedacore:main Aug 2, 2022
@tomkerkhove
Copy link
Member

Thanks a ton!

@tomkerkhove
Copy link
Member

Turns out there is a problem because the Redis scalers are not shown:
image

Also, can we remove the "*" from the message given this is internal details.

@tomkerkhove
Copy link
Member

I'll revert this PR because it also messed up our community page - Sorry!

https://keda.sh/community/#users

Before it was like this:
Screenshot_1

@thisisobate
Copy link
Contributor Author

it also messed up our community page

Was able to trace this issue. would add a fix to this branch and open as a separate PR

@tomkerkhove
Copy link
Member

Thanks! It's also OK to do a new PR as I didn't complete the revert yet

@thisisobate
Copy link
Contributor Author

@tomkerkhove My suggestion: I think it's best to open separate PRs for each issue you highlighted instead of reverting this PR. I already created a fix for each so it's safe to have them as follow-up PRs. Wdyt?

@tomkerkhove
Copy link
Member

👌

@tomkerkhove
Copy link
Member

tomkerkhove commented Aug 3, 2022

Turns out there is a problem because the Redis scalers are not shown: image

Also, can we remove the "*" from the message given this is internal details.

@thisisobate Just FYI that the main problem here is still open though. If you type in "Redis"/"Azure"/"AWS" it does not show any of our scalers.

Created #857

@thisisobate
Copy link
Contributor Author

Yeah...would take a look at it today

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline comments for questions and remarks. I know it's late, but better (a bit) late than never :)

layouts/_default/scaler-template.html Show resolved Hide resolved
layouts/partials/content.html Show resolved Hide resolved
config.toml Show resolved Hide resolved
const searchResultCount = document.querySelector(".results");
const template = document.getElementById("is-search-template");
let query = input.value.trim();
Copy link
Contributor

@chalin chalin Aug 15, 2022

Choose a reason for hiding this comment

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

This line seems to be the cause of:

because document.getElementById("search-input") can be null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide overview of all external scalers
8 participants