A4A Referrals: convert the actions field into proper actions #94250

oandregal · 2024-09-05T16:23:05Z

Follow-up to #93503
Related to #94198

Proposed Changes

This PR converts the existing actions field into proper DataViews actions.

Why are these changes being made?

Part of clean-up of DataViews in A4A to use core-first APIs.

Testing Instructions

yarn && yarn start-a8c-for-agencies

Visit agencies.localhost:3000 and navigate to the "Referrals" section.

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-09-05T16:23:18Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-118663

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-118663&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-118663&env=a8c-for-agencies

matticbot · 2024-09-05T16:32:57Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~12 bytes added 📈 [gzipped])

name                        parsed_size           gzip_size
a8c-for-agencies-referrals        -28 B  (-0.0%)      +12 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

client/a8c-for-agencies/sections/referrals/referrals-list/index.tsx

yashwin · 2024-09-06T04:01:33Z

Thanks for working on this!

I was asked to take a look at this here based on this pfYzsZ-g9-p2#comment-141

I tested the PR and found a few issues:

The chevron right icon is missing for the actions along with the ACTIONS column

Screenshot from this PR:

Screenshot from production:

Large screen:

Small screen:

I'm not sure if we are aware or reported already. I found this a bit confusing and feels redundant (View details action)

Please let me know your thoughts!

oandregal · 2024-09-06T16:21:42Z

The chevron right icon is missing for the actions along with the ACTIONS column

Thanks for taking a look! Yes, that's a known issue I mentioned in the PR description (TODO). Didn't get to work on this PR today but I will next week.

oandregal · 2024-09-09T08:41:20Z

client/hosting/sites/components/style.scss

@@ -49,10 +49,6 @@
 			padding: 0;
 		}
 	}
-	th[data-field-id="actions"] {


DataViews no longer includes data-field-id since 4.0.0 (see)

oandregal · 2024-09-09T08:45:38Z

client/a8c-for-agencies/sections/referrals/primary/referrals-overview/index.tsx

+	const [ dataViewsState, setDataViewsState ] = useState< DataViewsState >( {
+		...initialDataViewsState,
+		layout: {
+			primaryField: 'client',


This provides some styles (bolder, etc.), renders the field in a special place for list layout (comparing before/after, the field is rendered vertical aligned while before it was ). It also makes it not hidable in list layout and soon in table as well (see).

oandregal · 2024-09-09T08:46:42Z

client/a8c-for-agencies/sections/referrals/referrals-list/index.tsx

-								>
-									{ item.client.email }
-								</Button>
+								<span className="a4a-referrals-client">{ item.client.email }</span>


I wanted to make it look the same as before, so added this class to target it. Is this the proper naming structure?

matticbot · 2024-09-09T08:46:58Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/a4a-referrals-actions on your sandbox.

oandregal · 2024-09-09T08:47:39Z

client/a8c-for-agencies/sections/sites/sites-dataviews/style.scss

@@ -21,10 +21,6 @@
 		color: var(--color-jetpack-50);
 	}

-	.dataviews-view-table__actions-column {


I wonder if this style (and the other two in a8c-for-agencies & components/dataviews) is affecting any other screen.

Not as far as I've reviewed: A4A Sites, A4A Sites Import Modal, A4A Referrals Details, A4A Team, Dotcom Sites.

Also, DataViews doesn't render the actions column if no actions are declared. Not sure why this rule was necessary.

oandregal · 2024-09-09T08:47:55Z

client/a8c-for-agencies/style.scss

 	}

-	.dataviews-view-list li {
-		cursor: pointer;


This is already taken care of in dataviews.

oandregal · 2024-09-09T08:49:44Z

client/a8c-for-agencies/style.scss

-		cursor: pointer;
-	}
-
-	.dataviews-view-list li .components-h-stack {


Not sure what this was for, but it was creating some issues with vertical alignment and spacing in the list view when actions were visible.

oandregal · 2024-09-09T08:51:20Z

@Automattic/a4a-genesis This is now ready in terms of behavior/styling.

I need to review that there are not side-effects to other screens (Actions column is not unintentionally shown in other screens that are not ready for it, for example).

youknowriad · 2024-09-09T08:59:38Z

In my testing, the main behavior change I see is the fact that clicking the row (outside of the chevron) navigates to details in trunk but not in this branch.

Also in both this branch and trunk, when hovering a row (but not an action), there's an inconsistency between desktop and mobile: on mobile the cursor is a pointer but not in desktop.

youknowriad · 2024-09-09T08:59:57Z

I like the code cleanup a lot :)

oandregal · 2024-09-09T09:33:18Z

In my testing, the main behavior change I see is the fact that clicking the row (outside of the chevron) navigates to details in trunk but not in this branch.

👍

Also in both this branch and trunk, when hovering a row (but not an action), there's an inconsistency between desktop and mobile: on mobile the cursor is a pointer but not in desktop.

In production, I see the pointer both is desktop & mobile, but only when you hover over the client field, which was a button. In this branch, I don't see it in any breakpoint.

oandregal · 2024-09-09T09:34:22Z

One thing I've noticed is that the actions column is sticky but its background is transparent, hence the action buttons overlaps other columns when manually reducing the viewport. This also happens in trunk so it should be addressed separately.

oandregal · 2024-09-09T10:37:37Z

One thing I've noticed is that the actions column is sticky but its background is transparent, hence the action buttons overlaps other columns when manually reducing the viewport. This also happens in trunk so it should be addressed separately.

I've prepared a fix for this at #94323

yashwin · 2024-09-09T10:48:24Z

I tested the Referrals page.

The Actions menu with the View Details option on Referrals seems a bit repetitive. Maybe we could simplify it to enhance the user experience.

Is it possible to always show the chevron right icon and hide the action menu (... icon)? Something like below as in production:

In my testing, the main behavior change I see is the fact that clicking the row (outside of the chevron) navigates to details in trunk but not in this branch.

I also noticed this, are we going to fix this in another PR?

youknowriad · 2024-09-09T11:03:01Z

The Actions menu with the View Details option on Referrals seems a bit repetitive. Maybe we could simplify it to enhance the user experience.

@oandregal Can we hide the dropdown, if we only have primary actions?

oandregal · 2024-09-09T12:28:13Z

The Actions menu with the View Details option on Referrals seems a bit repetitive. Maybe we could simplify it to enhance the user experience.
...
Is it possible to always show the chevron right icon and hide the action menu (... icon)? Something like below as in production:
...
Can we hide the dropdown, if we only have primary actions?

No, that's how DataViews actions work.

Note that displaying only the primary actions if there are no secondary ones comes with some details to figure out: for example, only the dropdown is visible until an item is hovered to avoid visual weight. If we wanted to investigate this, it should be an upstream (core dataviews) follow-up PR.

I also noticed this, are we going to fix this in another PR?

This is intentional. The "click row" behavior should not be overriden (it's used for item selection with "bulk actions") and we should prefer primary actions for the "overview-to-details" interaction. See design feedback at pfYzsZ-g9-p2

youknowriad · 2024-09-09T12:30:32Z

Note that displaying only the primary actions if there are no secondary ones comes with some details to figure out: for example, only the dropdown is visible until an item is hovered to avoid visual weight. If we wanted to investigate this, it should be an upstream (core dataviews) follow-up PR.

Yes, that was actually my suggestion (upstream PR), maybe only when there's a single primary action and make it visible.

oandregal · 2024-09-09T16:54:07Z

Yes, that was actually my suggestion (upstream PR), maybe only when there's a single primary action and make it visible.

I don't think I can get to this this week, but created an issue in case anyone wants to tackle it (and also to gather design feedback): WordPress/gutenberg#65165

yashwin

Thanks for working on this!

I don't have any more feedback apart from the one I have mentioned already.

So, I'm approving the PR.

CC: @madebynoam @jeffgolenski FYI 🙂

oandregal · 2024-09-10T12:48:17Z

I'd like to wait until tomorrow to merge this PR, in case there's some late design feedback at pfYzsZ-g9-p2.

cleacos · 2024-09-11T09:58:27Z

I saw this issue:

In theory, it will be fixed by this PR: #94323

oandregal · 2024-09-11T11:05:31Z

I saw this issue:

That also happens in trunk:

Captura de ecrã 2024-09-11, às 12 01 34

In theory, it will be fixed by this PR: #94323

I can confirm it does. I've cherry-picked that branch into this one for local testing and this is the result:

Captura de ecrã 2024-09-11, às 13 05 19

oandregal added 2 commits September 5, 2024 18:19

Transform actions field into proper actions

1fc00d0

Remove unused styles

c48b921

oandregal requested review from vitozev and cleacos September 5, 2024 16:23

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 5, 2024

oandregal requested review from youknowriad and allilevine September 5, 2024 16:36

youknowriad reviewed Sep 5, 2024

View reviewed changes

client/a8c-for-agencies/sections/referrals/referrals-list/index.tsx Outdated Show resolved Hide resolved

rcanepa requested a review from a team September 6, 2024 02:13

rcanepa added the A4A label Sep 6, 2024

oandregal added the [Status] In Progress label Sep 9, 2024

oandregal added 7 commits September 9, 2024 09:32

Do not hide the actions column

608712e

Remove click row behavior

d964217

Make client the primary field

2e23e62

Remove styles that affect list

a7e05a1

Do not add actions in list view

84b00ab

Custom styles for client

a08c26a

Remove comment

487199f

oandregal commented Sep 9, 2024

View reviewed changes

oandregal removed the [Status] In Progress label Sep 9, 2024

oandregal marked this pull request as ready for review September 9, 2024 08:51

oandregal self-assigned this Sep 9, 2024

yashwin approved these changes Sep 10, 2024

View reviewed changes

oandregal mentioned this pull request Sep 10, 2024

Hosting dashboard: remove initial selected site hack #94295

Merged

7 tasks

oandregal merged commit fdb975c into trunk Sep 11, 2024
15 of 18 checks passed

oandregal deleted the update/a4a-referrals-actions branch September 11, 2024 11:30

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A4A Referrals: convert the actions field into proper actions #94250

A4A Referrals: convert the actions field into proper actions #94250

oandregal commented Sep 5, 2024 •

edited

Loading

github-actions bot commented Sep 5, 2024 •

edited by matticbot

Loading

matticbot commented Sep 5, 2024 •

edited

Loading

yashwin commented Sep 6, 2024 •

edited

Loading

oandregal commented Sep 6, 2024

oandregal Sep 9, 2024

oandregal Sep 9, 2024

oandregal Sep 9, 2024

matticbot commented Sep 9, 2024

oandregal Sep 9, 2024

oandregal Sep 9, 2024

oandregal Sep 9, 2024

oandregal Sep 9, 2024

oandregal commented Sep 9, 2024 •

edited

Loading

youknowriad commented Sep 9, 2024

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024

oandregal commented Sep 9, 2024

oandregal commented Sep 9, 2024

yashwin commented Sep 9, 2024

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024 •

edited

Loading

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024

yashwin left a comment

oandregal commented Sep 10, 2024

cleacos commented Sep 11, 2024

oandregal commented Sep 11, 2024

A4A Referrals: convert the actions field into proper actions #94250

A4A Referrals: convert the actions field into proper actions #94250

Conversation

oandregal commented Sep 5, 2024 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Sep 5, 2024 • edited by matticbot Loading

matticbot commented Sep 5, 2024 • edited Loading

yashwin commented Sep 6, 2024 • edited Loading

oandregal commented Sep 6, 2024

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal Sep 9, 2024

Choose a reason for hiding this comment

matticbot commented Sep 9, 2024

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal Sep 9, 2024

Choose a reason for hiding this comment

oandregal commented Sep 9, 2024 • edited Loading

youknowriad commented Sep 9, 2024

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024

oandregal commented Sep 9, 2024

oandregal commented Sep 9, 2024

yashwin commented Sep 9, 2024

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024 • edited Loading

youknowriad commented Sep 9, 2024

oandregal commented Sep 9, 2024

yashwin left a comment

Choose a reason for hiding this comment

oandregal commented Sep 10, 2024

cleacos commented Sep 11, 2024

oandregal commented Sep 11, 2024

oandregal commented Sep 5, 2024 •

edited

Loading

github-actions bot commented Sep 5, 2024 •

edited by matticbot

Loading

matticbot commented Sep 5, 2024 •

edited

Loading

yashwin commented Sep 6, 2024 •

edited

Loading

oandregal commented Sep 9, 2024 •

edited

Loading

oandregal commented Sep 9, 2024 •

edited

Loading