Actions: add Edit comment and reorder the icons

[Copons]

Fix #195 Require ~https://github.com/Automattic/wp-calypso/pull/22049/ and D9749-code~ (merged)

Add the Edit comment action and reorder the comment actions to be more consistent with Calypso's Comments Management section.

The Edit comment link can point to:

• https://wordpress.com/comment/{ siteSlug }/{ commentId } for WPCOM sites and JP sites with the edit_links_calypso_redirect option set.
• { adminUrl }/comment.php?action=editcomment&c={ commentId } otherwise.

Before	After

[Copons]

I've updated this PR to open the link via middleware. Also, the link is now generated by the server (in D9749-code), and the client now just have to follow it and call it a day.

[kwight]

Tested this as-is; redirects worked as expected for WP.com and edit_links_calypso_redirect=false, but I was still sent to the JP site's WP Admin when I set edit_links_calypso_redirect=true (I did this with wpsh on the shadow site). Ignore me here, I was testing incorrectly 🙃

I noticed that when we land in Calypso, it's not really obvious what a user should do (they still have to click Edit again). Did we not set up a query param at some point that loaded the view with the Edit section open?..

[Copons]

I noticed that when we land in Calypso, it's not really obvious what a user should do (they still have to click Edit again). Did we not set up a query param at some point that loaded the view with the Edit section open?..

@kwight You're right, there indeed is the ?action=edit param that forces the comment to load in edit mode. Though, when starting in edit mode, it's not really obvious what a user should do if they want to change the comment status: they would have to submit/cancel the form to exit edit mode.

IIRC there has been some discussion about this involving @rodrigoi, but I couldn't remember where. In fact, I've been meaning to raise this question, and I forgot about that too. 🤦‍♂️

[Copons]

@dmsnell @rodrigoi Hope I did everything right.

In 1e394c3 I've converted the edit comment middleware from a simple window-opener into something that also sends the comment details via sendMessage.

In the 32464 revision to D9749-code, I've added an editComment tracker in masterbar-tracks.js. In https://github.com/Automattic/wp-calypso/pull/22049/, I've added an EDIT_COMMENT tracker in Calypso.

[kwight]

@Copons hm, although, that does bring up an interesting point: why do we treat Edit and Reply differently? Reply drops down, leaving actions available, while Edit covers up the actions. Any reason Edit couldn't drop down like Reply also? cc/ @drw158

[Copons]

I guess it's an established UX that inline editing replaces the content with a textarea (see Facebook, for example). Non-inline (meaning: by opening a new page) editing too, is pretty much the same thing but on different pages.

Like, having the old rendered content and the edit textarea both visible at the same time is sort of never heard before.

Usually, if there is a rendered content along the edit textarea, the content is a preview of the edits. So I guess, we could either work on a WYSIWYG comment editor (:nopetopus:) or keep textarea and render well separated.

[davewhitley]

I think the best course of action would be to show the edit state if they click on Edit in the notification. This is because the other actions are available to them as well in the notification.

It's not super ideal, and there could be some confusion. Does the Edit button in notifications go to the single comment view?

[kwight]

@drw158 It does, yes. But that seems reasonable?..

[gwwar]

@drw158 @Copons Are we mostly blocked here by behavior questions?

Thinking about this again, one thing I'd point out is that we aren't editing inline, so we'd likely want to have a external link icon that hints that you'll be taken away from your current view. I think the more ideal behavior here is to allow simple inline editing in Notifications, since we also allow you to reply inline.

Could we perhaps add a clearer option to link to "manage comments" within the note content? This seems to be what we originally suggested in https://github.com/Automattic/notifications-panel/issues/195

[kwight]

Thinking about this again, one thing I'd point out is that we aren't editing inline, so we'd likely want to have a external link icon that hints that you'll be taken away from your current view.

I understand that thinking, but do we consider necessary? Even though this repo is a separate module, our primary use of within Calypso means most users won't be changing domains when clicking the Edit link.

I think the more ideal behavior here is to allow simple inline editing in Notifications, since we also allow you to reply inline.

I wonder if that's a bigger leap for another time, deserving more UX and design investigation of its own? I still feel the best way to move this forward (meaning, what's the next step we can take in the process), is to add the ?action=edit and merge what we have here.

[gwwar]

Even though this repo is a separate module, our primary use of within Calypso

@kwight Are you forgetting about the Masterbar that's in use across all wp.com sites when logged in? You may be reading some other blog at that point, before being redirected to Calypso. :)

[kwight]

Are you forgetting about the Masterbar that's in use across all wp.com sites when logged in?

Doh! Yes, I may have forgotten 🙃 Great point.

[kwight]

It looks like next actions on this to make it happen would be:

• add the ?action=edit param, and
• add an external URL icon to the Edit button.

Sounds good? @Copons do you want to pick this back up?

[Copons]

do you want to pick this back up?

Yes! I've updated D9749-code with the ?action=edit param.

Though, it's unclear to me how we want to add the external icon to the Edit button:

• Add it as a "badge" (white icon on gray background, bottom-right corner), like the notification type icons that appear on the notification author avatar?
• Replace the pencil icon with the external one altogether?

cc @drw158

[davewhitley]

Can we show the edit icon for ppl that are in Calypso?

For ppl outside, I like your idea of just using an external icon instead of the pencil.

You might consider using this icon instead of the external icon:

I think the best way to handle it is to not open a new tab/window. Open in the same window.

[Copons]

Moved back to WIP for a doubt that might end up dropping the last commit.

---

@drw158 So, with 8730651 we do exactly what you asked: if we are inside Calypso, show pencil; otherwise popout.

Though, I realized that we might be in the opposite situation. What if we are inside Calypso but the edit link points to wp-admin (I don't remember why this can happen, but it's a possible case)? And what if the edit link points to wp-admin and we in fact are inside that wp-admin?

This quickly becomes very hard (aka overkill) to figure out, unless we want to disregard some of those as edge cases.

All in all, though, I'd maybe drop 8730651 and simply replace pencil with popout, and open into _blank for all cases.

[davewhitley]

Hm I can't think of a reason why edit would point to WP Admin. If there is a strange edge case, that's ok if the icon is pencil.

So, if you are in WP Admin, I think it should show the popout icon. It should treat WP Admin as an external area. Are you saying that would be hard to do? I think that might be a pretty common case for Jetpack users 😨

All in all, though, I'd maybe drop 8730651 and simply replace pencil with popout, and open into _blank for all cases.

That'd be ok for now, but hopefully we could detect if you are in Calypso!

[kwight]

I think we're getting stuck on Calypso or not, Jetpack or not; this repo can be used by anyone for any project, so really it's just about what icon do you want for the Edit link: pencil or popout. The Jetpack/not Jetpack stuff brings up the point though that we may need to be able to do this on a note by note basis, so isExternal would be a prop on individual actions (needn't be just Edit really).

Total side note: I was assuming the big icon would always be the pencil, and we would have a little external URL icon beside the label, more similar to what we do in other places like WP Admin, more like this:

[Copons]

Ok, I've dropped the logic that check if we are in Calypso and for now we will fall back on a single Edit button (pencil icon; _blank target) for all cases; in the future though we'll likely have inline editing.

[kwight]

Awesome work @Copons ! It all looks and works great. Let's get this merged, and continue debate of external URL handling in #238 .

[folletto]

Note for the future: mobile teams should have been notified of this change as now iOS and Android are misaligned.

Conversation here: p4a5px-2bT-p2

[gwwar]

Thanks for the reminder @folletto! It totally slipped our minds that the notifications-panel is not consumed by mobile clients.

[kwight]

Yes, thank you @folletto ! I've updated the README and internal docs with a reminder 👍

[folletto]

@kwight excellent idea! Maybe we can phrase it as "open a corresponding issue on the iOS and Android repositories"? These are open source so anyone can open issues there ;)

[kwight]

@folletto done! https://github.com/Automattic/notifications-panel/commit/80763eb0144f86d56d6e8abd80eb71539184a947

[folletto]

Thank you! 🌟