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

Fixes #35012 - add call-to-action empty states #10144

Conversation

tazhibaevaaliya
Copy link
Contributor

What are the changes introduced in this pull request?

Added call-to-actions for empty state on Host collections (under Host Overview Tab), Module Streams (under Content Tab), Repository sets, and Host content view selection (under Host Overview Tab -> Content View Details) pages/tabs

Considerations taken when implementing this change?

I believe that adding call-to-actions will increase user experience whenever they are no existing/available items/products/repos to show

What are the testing steps for this pull request?

For all the corresponding tabs:

  1. Navigate to the corresponding tab/page
  2. Assuming that you do not have available/existing items to show, you should see action button(s) (the number of action buttons depends on different number of the ways you can create a specific item/product/repo), which should direct you to the creation page
  3. Try adding some items and searching for non-existing item. You should see a message indicating that such item does not exist without call-to-action button(s)

Note: there is a separate card created for the last action item (empty search)

@theforeman-bot
Copy link

Can one of the admins verify this patch?

@theforeman-bot
Copy link

Issues: #35012

@jeremylenz
Copy link
Member

[test katello]

@tazhibaevaaliya tazhibaevaaliya force-pushed the 35012-add-call-to-action-empty-states branch from 08ef72b to 8b13871 Compare June 7, 2022 16:36
@tazhibaevaaliya
Copy link
Contributor Author

Here are the screenshots of the final result on all four pages:
Screenshot from 2022-06-07 13-05-11
Screenshot from 2022-06-07 13-05-06
Screenshot from 2022-06-07 13-04-54
Screenshot from 2022-06-07 13-04-31

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Thanks @tazhibaevaaliya!

I'll let @lfu do the testing etc. but wanted to leave some code comments below.

Also, seems like the React tests will need some work!

webpack/components/Table/EmptyStateMessage.js Outdated Show resolved Hide resolved
webpack/components/Table/EmptyStateMessage.js Outdated Show resolved Hide resolved
webpack/components/Table/MainTable.js Outdated Show resolved Hide resolved
webpack/components/Table/TableWrapper.js Outdated Show resolved Hide resolved
@lfu
Copy link
Member

lfu commented Jun 7, 2022

Seems duplicated @jeremylenz's comments :)

Tests looked well for RHEL hosts.

But Debian hosts would get the same page for Repository sets and Module streams which seem not appropriate. Any thought? @jeremylenz

@jeremylenz
Copy link
Member

But Debian hosts would get the same page for Repository sets and Module streams which seem not appropriate. Any thought?

Hmm.. We could maybe check if the deb content type is enabled and not show the RH Repos link if so? @ianballou would that even work? I would consider it extra credit either way.

@tazhibaevaaliya
Copy link
Contributor Author

@jeremylenz @lfu What is the best way to incorporate your comments? Should I manually add and make another commit or there is an easier way?

@jeremylenz
Copy link
Member

If you want to keep it squashed, you can make your changes and then

git add .
git commit --amend --no-edit

and force push.

If you want to add a commit instead, you can just add it and then do a regular push. I'm fine with either one 👍

@ianballou
Copy link
Member

ianballou commented Jun 7, 2022

Hmm.. We could maybe check if the deb content type is enabled and not show the RH Repos link if so? @ianballou would that even work? I would consider it extra credit either way.

Debian is installed by default, so likely few upstream users would see this RH Repos link then.

Is there a fact or something that we could grab from the API to determine that the host is a RHEL one?

At least in Rails you can do ::Host.find(<id>).os which returns a Redhat object for my RHEL 7 host. Not sure if we expose that in the API.

Edit: on the content host page I see "OS" under "Content Host Properties", so there must be a way.

@jeremylenz
Copy link
Member

Looks like the host details provides operatingsystem_name property:

operatingsystem_id: 4
operatingsystem_name: "RedHat 8.5"

We could see if that string contains "RedHat". Kinda hacky, but would save us another API request. thoughts?

@tazhibaevaaliya tazhibaevaaliya force-pushed the 35012-add-call-to-action-empty-states branch from 8b13871 to 9bcd988 Compare June 7, 2022 20:45
@ianballou
Copy link
Member

We could see if that string contains "RedHat". Kinda hacky, but would save us another API request. thoughts?

Here's the old logic for determining if a host is a Debian one: https://github.com/Katello/katello/blob/master/engines/bastion_katello/app/assets/javascripts/bastion_katello/hosts/host.factory.js#L30

@jeremylenz
Copy link
Member

Love it 😆

@tazhibaevaaliya
Copy link
Contributor Author

@ianballou @jeremylenz @lfu Will we need to create a separate card for this issue then?

@tazhibaevaaliya
Copy link
Contributor Author

Also, quick question, once I committed, should I rebase once more to pick up the latest commit in my emptySearch/35027-add-link-to-clear-search branch? @jeremylenz

@jeremylenz
Copy link
Member

should I rebase once more to pick up the latest commit in my emptySearch/35027-add-link-to-clear-search branch?

It's up to you. You'll have to fix any conflicts either way -- you're just choosing to do it later, when this PR is merged, or now if you decide to rebase. :)

@tazhibaevaaliya
Copy link
Contributor Author

tazhibaevaaliya commented Jun 7, 2022

@jeremylenz Yay! I just love rebasing 😄

@tazhibaevaaliya tazhibaevaaliya force-pushed the 35012-add-call-to-action-empty-states branch 2 times, most recently from 41f71b9 to 10ea804 Compare June 8, 2022 16:03
@lfu
Copy link
Member

lfu commented Jun 8, 2022

APJ

@jeremylenz
Copy link
Member

[test katello]

@tazhibaevaaliya tazhibaevaaliya force-pushed the 35012-add-call-to-action-empty-states branch from 10ea804 to dbb6b8f Compare June 9, 2022 17:07
@jeremylenz
Copy link
Member

[test katello]

Copy link
Member

@lfu lfu left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Great work @tazhibaevaaliya!

ACK 👍

@tazhibaevaaliya
Copy link
Contributor Author

tazhibaevaaliya commented Jun 10, 2022

@jeremylenz and @lfu Thank you for all your help!

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