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: [M3-6427] - Add No Results section for Marketplace Search #8999

Merged
merged 9 commits into from
Apr 19, 2023

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Apr 13, 2023

Description 📝

Add No Results section for Marketplace Search

Preview 📷

image

Remove this section or include a screenshot or screen recording of the change

How to test 🧪

  • Navigate to Marketplace search
  • Search for app that doesn't exists (ex; datadog)
  • Should display "No Results" message.

How do I run relevant unit or e2e tests?

  • yarn test
  • yarn cy:run

@jdamore-linode
Copy link
Contributor

Nice, it works well!

Would it be possible to center the "No Results" text?

Screen Shot 2023-04-13 at 4 11 29 PM

Do we need any other input from the UX team as far as wording and design should go?

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

I'm glad we're doing this! Seconding @jdamore-linode's request to center the text horizontally. And I'm wondering about the overall placement of the text within that container.

Mock (assume actual center alignment):
Screenshot 2023-04-13 at 3 09 00 PM

@cypress
Copy link

cypress bot commented Apr 14, 2023

1 flaky tests on run #3058 ↗︎

0 149 3 0 Flakiness 1

Details:

Update CHANGELOG.md
Project: Cloud Manager E2E Commit: bd23e1e8a5
Status: Passed Duration: 15:40 💡
Started: Apr 19, 2023 3:33 PM Ended: Apr 19, 2023 3:48 PM
Flakiness  cypress/e2e/domains/smoke-create-domain-records.spec.ts • 1 flaky test

View Output Video

Test Artifacts
Creates Domains record with Form > Add an A/AAAA Record Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@cpathipa
Copy link
Contributor Author

Thank you @jdamore-linode and @mjac0bs thank you for the feedback, I will circle back with UX on this.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

I also agree with checking on the centered-within-container option

@cpathipa
Copy link
Contributor Author

Thank you for the feedback. Based on the UX feedback, updated the no results copy and adjusted alignment to center.
image

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Thoughts on left aligning the no results section? The selection cards are left aligned as well as the "no compatible images" text below

Do we also need to include "sorry" in the text?

image

@mjac0bs
Copy link
Contributor

mjac0bs commented Apr 17, 2023

Nitpicks on copy: I'm wondering if clearer copy might read: "Sorry, no results matched your search for "datadog". Please try another term."

Thought process being:

  1. "no [search term] results" sounds kind of strange and is slightly harder to scan as a reader than "no results"
  2. telling the user to "please try again" feels misleading because if the user were to try the same action again, the results would be the same. It feels more clear to state what they should actually try to yield different results.

What do others (and UX) think?

@hana-akamai
Copy link
Contributor

@mjac0bs Agreed that "Sorry, no results matched your search for "datadog" sounds clearer. I don't think we even need the second sentence as it's pretty obvious that the user should search for a different term

@cpathipa cpathipa requested a review from hana-akamai April 18, 2023 15:19
@jaalah-akamai
Copy link
Contributor

Let's remember to update changelog too 👍

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Just a changelog update and this looks like it's good to go.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Apr 19, 2023
@cpathipa cpathipa merged commit 00e37d2 into linode:develop Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Marketplace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants