Conversation
Enable the A4A dashboard entrypoint in dashboard development with hostname support, server routes, and agency/client surfaces. Made-with: Cursor
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
Dashboard Live (dotcom) (direct link)
|
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
p-jackson
left a comment
There was a problem hiding this comment.
Nice, it's a good start! Looks like you've found the main places where configuration happens. My main encouragement is to use things like if ( appConfig.name === 'A4A' ) {} very sparingly and to try and think about how a different product based on subscriptions (think JP cloud) might also use the features you're creating.
Something that hasn't been modified in this PR yet (which is fine) is the site overview page (/sites/$siteSlug). That is the one place where we do have a lot of "if dotcom" and "if ciab" logic since they each want different cards. Bringing A4A into the MSD could be the opportunity to change this a bit. Have 3 separate overview page components
<DotComSiteOverview><CiabSiteOverview><PressableSiteOverview>
Which all compose themselves out of the same grid component building blocks for consistency, but have freedom about which cards they want to show.
Anyway, just pondering.
| <p> | ||
| { __( | ||
| 'Minimal placeholder for the A4A agency client subscriptions overview.', | ||
| 'full-site-editing' |
There was a problem hiding this comment.
Just wondering here whether you would expect the a4a strings to exist in a different translation domain from the rest of MSD?
| * Temporary location. | ||
| * | ||
| * This is dashboard-local while A4A routing/entrypoints/auth are still evolving. | ||
| * Once stabilized, we should move the query builder into `@automattic/api-queries` |
There was a problem hiding this comment.
I think you should feel confident putting your queries into the package even while they're still stabilising. The package isn't published to npm, and we've been freely tweaking the queries in that package as we go.
The reason I'd prefer the queries in their is that this would introduce a new data/ folder and I worry it would being to be used in ways that we don't want.
| * We use the standard dashboard auth flow (`wpcom_token`) rather than the legacy | ||
| * A4A-specific token (`wpcom_token_a4a`) to avoid OAuth loops in dashboard environments. | ||
| */ | ||
| export async function wpcomA4ARequest< T >( |
There was a problem hiding this comment.
Is there something special about these types of requests that mean they can't be in @automattic/api-core? lib/wp is where we change the request logic based on whether we are using cookies or oauth, perhaps any special tokens related to a4a would belong in lib/wp rather than here?
|
|
||
| export const A4A_CONTEXT_QUERY_KEY = [ 'a4a', 'agency-context' ] as const; | ||
|
|
||
| export function useA4AContextQuery( enabled: boolean ) { |
There was a problem hiding this comment.
A couple points about this request hook.
It might be worth taking a look at
to see how we're using request objects instead of hooks throughout MSD. We've found that request objects scale much better, since sometimes you wantuseQuery, sometimes useSuspenseQuery, sometimes ensureQueryData, etc. etc. And that those decisions are best left up to the call site.
The other is that I worry about a request specifically called "a4a context". We've tried to prevent so much branching logic throughout the code by removing things like "if ciab", "if dotcom", etc. and I worry that something called "a4a context" would result in that sort of branching within the product. Is the data truly only applicable to A4A? Things like is_client_user and has_agency seem like they're related to the logged in user and so would it make sense to incorporate these things into the user data?
|
|
||
| export default function AgencyMenu() { | ||
| const { user } = useAuth(); | ||
| const { data: a4aContext } = useA4AContextQuery( Boolean( user ) ); |
There was a problem hiding this comment.
This the sort of place where we would use useSuspenseQuery, since rendering can't continue until the data has finished loading.
| import { useAuth } from './auth'; | ||
| import { useAppContext } from './context'; | ||
|
|
||
| export default function A4AContextProvider( { children }: { children: React.ReactNode } ) { |
There was a problem hiding this comment.
I'm not sure what exactly this A4AContextProvider is providing 🤔
| Logo: React.FC | null; | ||
| LoadingLogo?: React.FC; | ||
| supports: { | ||
| agency: boolean; |
There was a problem hiding this comment.
I'm not sure whether we should have two booleans here, one for agency overview and one for the client subscription page 🤔 I see how they sort of go together, but we've also always had a 1:1 match of these AppConfig flags per menu item.
| - Shared core functionality | ||
|
|
||
| Currently, the dashboard supports two main entry points: | ||
| Currently, the dashboard supports multiple entry points: |
There was a problem hiding this comment.
| Currently, the dashboard supports multiple entry points: | |
| The dashboard supports multiple entry points: |
|
This might be a conversation for another PR, but @yashwin if you take a look at Do you think that A4A is likely to be a "site type" according to |
Proposed Changes
entry-dashboard-a4a) with its own hostname (my.a4a.localhost) and A4A-specific routes./overviewand/client/subscriptions) so direct hits/refresh work./agencyis fetched and cached.Why are these changes being made?
/a4aandmy.a4a.localhost.Testing Instructions
Note: You should have an A4A account: https://agencies.automattic.com/signup
yarn start-dashboard.http://my.a4a.localhost:3000/./overviewand hard-refresh; confirm it still loads (no "Cannot GET /overview")./client/subscriptionsand hard-refresh; confirm it still loads.Pre-merge Checklist
Made with Cursor