-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Gutenboarding: Add domain categories. #41358
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~1968 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
{ paidSuggestions && | ||
( paidSuggestions?.length ? ( | ||
paidSuggestions.map( ( suggestion, i ) => ( | ||
<div className="domain-picker__body"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't be alarmed by lots of diffs, all the changes in this file is just to nest the divs in an outer div .domain-picker__body
, code remains the same.
As a quick note on fetching the categories, I have a prototype diff for this in D42138-code. You should be aware that from a data structure perspective that we will need both the localized display name and a key/slug to include in the API calls to the domain suggestions end point. (The additional parameter to the end point doesn't exist yet, but I wanted to get feedback on this approach first.) More discussion in #41081. |
5ab9ed3
to
3b3d39f
Compare
Use stateful shownDomain Ensure button (and modals) are persisted across search
4351a5c
to
2fa50a8
Compare
rebased |
Will be handled in #41450 / D42138-code (not yet ready)
We can iterate on this when and if it becomes a feature of the domains API. It does seem redundant to pass the TLD list back and forth with the client. (cc: @daledupreez)
Will be handled in #41450 / D42138-code (not yet ready)
It seems problematic that we're putting all of the domain search state into the onboarding store (search query/categories). Onboarding store state has also permeated the domain picker, initially we were treating it like a library that should not depend on store state but that has been lost. I'd like to work to recover the domain-picker as a library, but in the short term we can continue as we are now.
Popover doesn't have the concept of categories, I don't believe they should be reflected there. The apparent, visible query should be shown.
Looking at an API query to a
That appears to be some preferred domains, followed by an alphabetical list? Maybe I expect #41450 / D42138-code will solve this.
✅ I think we're good here with recent changes.
I've followed the pattern used elsewhere - hover is like selected state. Focus has extra visual queues with keyboard navigation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big changeset. It's not perfect, but I'd like to get it landed so we can improve it in smaller PRs.
Nice work on the bulk of this @yansern 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE the open issues/questions:
1, 2, 3, and 6 should all be addressed by a combination of:
- Gutenboarding: Use domain categories from API. #41450 and D42138-code to get a sortable list of translated category titles and slugs/identifiers
- TBD and D42138-code to support the category slug/identifier as a parameter on the suggestions API
We should be able to avoid dealing with the complete list of TLDs.
client/landing/gutenboarding/components/domain-categories/index.tsx
Outdated
Show resolved
Hide resolved
*/ | ||
import type { ValuesType } from 'utility-types'; | ||
|
||
export const domainTldsByCategory = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be very nice to drop this once we land the category API support!
Changes proposed in this Pull Request
Testing instructions
Note: This is hiding under a feature flag so it is only testable locally.
/new
.Known issues / Things To Consider
A proper way to load the list of domain categories?
The
domains.ts
is just a temporary workaround. https://github.com/Automattic/wp-calypso/blob/5ab9ed3cd0df76e879bf863543cdd5aee54f1f64/client/landing/gutenboarding/domains.tsAvoid tlds prop?
There might not be a need to use the
tlds
prop in the future if domains API accept domain category param.How to translate domain category string?
If we find out how the list of domain categories are loaded (item no. 1), we'll find an answer to this one to.
(Semi-related) Quantity for modal should be 10, quantity for popover should be 5.
I was considering introducing the
quantity
prop to theDomainPicker
component when I noticedqueryParameters
prop is not used. ShouldqueryParameters
prop be passed over touseDomainSuggestions()
?Should domain category in popover view reset back to Popular after user changed it in the modal view?
The "Popular" domain category?
What tlds should the "Popular" domain category include? Currently I just simply added
.com
,.net
and.org
. Or if "Popular" is chosen, no TLDs should be passed to the API and the API will return popular TLDs?Mobile view of categories not implemented.
Focus outline seems to be clipped.
Need design input on how category button should look like when hovered & clicked.
Screenshot
Fixes #41081