Skip to content

New marketplace search component #39149

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.

Sign up for GitHub

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

Jump to bottom
Merged
andfinally merged 1 commit into from
Jul 24, 2023
Merged

New marketplace search component #39149

andfinally merged 1 commit into from
Jul 24, 2023

Conversation

@mlaetitia
Copy link
Contributor

@mlaetitia mlaetitia commented Jul 7, 2023 •
edited by kdevnel
Loading

Submission Review Guidelines:

Changes proposed in this Pull Request:

Add a new search component to the new marketplace react page (under WooCommerce WP Admin).
This component is prepared to call the WooCommerce marketplace API when a search query is entered.

We still need to add locale and country to the query as Props when calling the component.

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Checkout this branch and build wc admin on your local environment
  2. Add the component to a page . I tested it by:
  • Adding import Search from '../search/search'; to plugins/woocommerce-admin/client/marketplace/components/content/content.tsx
  • Add return <Search> instead of return <Extensions> on line 22
  1. Open WP admin -> WooCommerce -> Marketplace and head to the place where you added the component. Confirm the new component is there and looks like the project design
  2. IMPORTANT: Might be a good idea to add a div with a certain width around the <Search> component - I purposely did not set the width to it so it would adapt easily to different layouts.
  3. Add a debugger;. line to runSearch in plugins/woocommerce-admin/client/marketplace/components/search/search.tsx to confirm that it's triggered both when clicking the magnifying glass icon and when hitting Enter on the keyboard.

Changelog entry

  • Automatically create a changelog entry from the details below.
Details

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement

Message

Comment

@mlaetitia mlaetitia self-assigned this Jul 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023 •
edited
Loading

Test Results Summary

Commit SHA: d26ede8

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 51s
E2E Tests1890019020818m 9s
To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023 •
edited
Loading

Hi @Dan-Q, @andfinally, @simranvijwani,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Hi @Dan-Q, @andfinally, @kdevnel, @simranvijwani,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@mlaetitia mlaetitia marked this pull request as ready for review July 7, 2023 16:16
simranvijwani
simranvijwani reviewed Jul 10, 2023
View reviewed changes
Copy link
Contributor

@simranvijwani simranvijwani left a comment

Choose a reason for hiding this comment

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

I haven't tested the PR yet, but based on my observation of the code, I recommend using variables in CSS.
Check out this example CSS file from the woocommerce-admin plugin where we utilize variables. It seems like we have to use variables from the Gutenberg base style. We have a stylesheet with available color variables here.

@kdevnel kdevnel force-pushed the add/marketplace-react-skeleton-layout branch from 979e7d0 to 4d3d663 Compare July 12, 2023 10:07
kdevnel
kdevnel suggested changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@kdevnel kdevnel left a comment

Choose a reason for hiding this comment

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

This seems to work as described in your testing. Just noting that the design and location of the component has changed quite a lot since you opened this for review so I've requested changes for that reason only.

@mlaetitia
Copy link
Contributor Author

mlaetitia commented Jul 13, 2023

@simranvijwani @kdevnel
Merged with the parent's new skeleton and used the variables.scss new file we now have, with all color codes as variables. Could you re-check? Thanks!

kdevnel
kdevnel approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@kdevnel kdevnel left a comment

Choose a reason for hiding this comment

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

Looking great, I'm going to approve this one. There are some linting errors in the automated checks. I've suggested fixes for them for now.

@mlaetitia
Copy link
Contributor Author

mlaetitia commented Jul 14, 2023

Thanks @kdevnel - made all the changes to fix linters, only the changelog missing, but definitely agree we can do a big Feature Branch one at the end.
All is good - assigning this to you now!

@mlaetitia mlaetitia assigned kdevnel and unassigned mlaetitia Jul 14, 2023
Dan-Q
Dan-Q suggested changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@Dan-Q Dan-Q left a comment

Choose a reason for hiding this comment

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

One minor a11y change suggested (type="search").

One other comment (a11y/progressive) that I don't think's worth acting on but noting for educational purposes.

@kdevnel kdevnel requested a review from Dan-Q July 19, 2023 11:20
@kdevnel
Copy link
Contributor

kdevnel commented Jul 19, 2023

@Dan-Q I've implemented your requested change now. Thanks for the review!

@Dan-Q Dan-Q mentioned this pull request Jul 19, 2023
Merged
@andfinally andfinally force-pushed the add/marketplace-search-component branch from 46eafdf to d5ba426 Compare July 24, 2023 15:13
@andfinally andfinally changed the base branch from add/marketplace-react-skeleton-layout to add/marketplace-skeleton-branch2 July 24, 2023 15:14
@andfinally andfinally force-pushed the add/marketplace-search-component branch from c7c0262 to acc23fc Compare July 24, 2023 15:55
@andfinally
In-app marketplace search component. Cleaned up the commit history of…
d26ede8 
… this branch. Copied changes from these commits:

e982842
9ca2ae3
e478157
976811c
46eafdf

Deleted unused import.
@andfinally andfinally force-pushed the add/marketplace-search-component branch from acc23fc to d26ede8 Compare July 24, 2023 16:06
@andfinally andfinally merged commit ffe99a5 into add/marketplace-skeleton-branch2 Jul 24, 2023
@andfinally andfinally deleted the add/marketplace-search-component branch July 24, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andfinally andfinally Awaiting requested review from andfinally

@simranvijwani simranvijwani Awaiting requested review from simranvijwani

2 more reviewers

@kdevnel kdevnel kdevnel approved these changes

@Dan-Q Dan-Q Dan-Q approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kdevnel kdevnel

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mlaetitia @kdevnel @Dan-Q @simranvijwani @andfinally