Skip to content
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

Update meta-box-subscription-info.php #180

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

remcotolsma
Copy link
Member

@remcotolsma remcotolsma self-assigned this Jun 3, 2024
Copy link
Member

@rvdsteege rvdsteege left a comment

Choose a reason for hiding this comment

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

It works, but looks a bit messy to me:

Scherm­afbeelding 2024-06-03 om 15 40 15
  • 4 times the word "links" in titles
  • all descriptions start with the phrase "This link can be shared with the customer", which also happens to be the same text as the "Action links" help tooltip
  • right alignment of the button makes the button jump when "Copied!" disappears again
  • URLs can be quite long, resulting in multiple lines, can we maybe truncate them someway without ending up with three equal URLs from the start of the URLs (e.g. all https://pay.test/?subscription=43653&key=sub...)?

Would a single description for all actions with clear titles not work, too?

Scherm­afbeelding 2024-06-03 om 16 19 52

Comment on lines 308 to 314
.pronamic-pay-action-link {
display: flex;

flex-direction: column;

gap: 5px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? Doesn't seem to have much effect on the styling, except for some random gap spacing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Random gap spacing? It's a fixed gap of 5px? It can also be done with margin/padding, I thought this was flex 😏

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, with ‘random gap’ I meant to say that a regular paragraph also adds some spacing if needed?

views/meta-box-subscription-info.php Outdated Show resolved Hide resolved
@remcotolsma
Copy link
Member Author

Did some small improvements in ca1f781.

Scherm­afbeelding 2024-06-03 om 19 52 43

4 times the word "links" in titles

Is that a problem?

all descriptions start with the phrase "This link can be shared with the customer", which also happens to be the same text as the "Action links" help tooltip

Is that a problem?

right alignment of the button makes the button jump when "Copied!" disappears again

Improved in ca1f781.

URLs can be quite long, resulting in multiple lines, can we maybe truncate them someway without ending up with three equal URLs from the start of the URLs (e.g. all https://pay.test/?subscription=43653&key=sub...)?

Not easy?

Would a single description for all actions with clear titles not work, too?

It is a pity that the explanation per link is lost, this one in particular may be valuable:

This link can be shared with the customer and gives the customer the opportunity to change the payment method. This is useful if a credit card expires or if a customer wants to have the charge debited from another account.

@rvdsteege
Copy link
Member

4 times the word "links" in titles

Is that a problem?

I wouldn't call it a "problem", but it looks a bit odd and I think that the title on the left side and blue underlining already make it clear enough that these are "links".

all descriptions start with the phrase "This link can be shared with the customer", which also happens to be the same text as the "Action links" help tooltip

Is that a problem?

Yes, repetitive text makes it harder to differentiate between the purposes of the links when scanning the descriptions.

URLs can be quite long, resulting in multiple lines, can we maybe truncate them someway without ending up with three equal URLs from the start of the URLs (e.g. all https://pay.test/?subscription=43653&key=sub...)?

Not easy?

Maybe server-side then? (I can't imagine anyone ever going to select the URL to copy it, instead of copying via the context menu or copy button)

$url = 'https://www.example.com/?subscription=43653&key=subscr_663b8737108d0&action=cancel';

echo \sprintf(
    '%s...%s',
    \substr( $url, 0, 32 ),
    \substr( $url, -24 )
);

// https://www.example.com/?subscri...b8737108d0&action=cancel

It is a pity that the explanation per link is lost, this one in particular may be valuable:

This link can be shared with the customer and gives the customer the opportunity to change the payment method. This is useful if a credit card expires or if a customer wants to have the charge debited from another account.

Therefore I added "update" to the title, as users might not think of updating an expired card or current account as "changing" the payment method. But we could leave the last emphasised part to clarify these specific use cases? Moving the description before the link looks a bit more balanced IMO (especially now that there's more whitespace next to the copy button on its own line).

@kjtolsma
Copy link
Member

kjtolsma commented Jun 4, 2024

This implementation makes it a quite massive block. I don't think it needs that much attention. Some additional suggesties.

  • Use tooltips for the descriptions?
  • "Copy URL to clipboard" is ok but if i see this i think an icon would be a better choice.

URLs can be quite long, resulting in multiple lines, can we maybe truncate them someway without ending up with three equal URLs from the start of the URLs (e.g. all https://pay.test/?subscription=43653&key=sub...)?

Maybe use an input field after all? It will solve this issue and makes more sense in my opinion.

https://github.com/pronamic/pronamic-pay/issues/99

@remcotolsma
Copy link
Member Author

remcotolsma commented Jun 4, 2024

Did some adjustments in 9a9a6de.

Scherm­afbeelding 2024-06-04 om 13 29 04

I got inspiration from Woo Subscriptions:

Scherm­afbeelding 2024-06-04 om 13 36 41

Maybe use an input field after all? It will solve this issue and makes more sense in my opinion.

I have already given reasons why I don't think this is ideal. Have you already considered that? Perhaps my objections can be resolved by adding a 'View page' button in addition to a copy button? Does that perhaps give us the best of both?

Scherm­afbeelding 2024-06-04 om 13 44 58

@kjtolsma
Copy link
Member

kjtolsma commented Jun 4, 2024

I got inspiration from Woo Subscriptions:

I don't know if this is the best inspiration. It is a bit of confusing link. I already created an issue for that: https://github.com/pronamic/pronamic-pay/issues/97. We are now introducing the same issue here 😉.

I think this is much better:

image

But still not sure about the "View page" button. The primary task of the element is "Sharing the action links" with the customer. The copy button adds value for that. I don't think the "View page" button does. You can simply copy past the URL the browser to view the link if you want. Keep things clean and lean. Just my opinion.

@remcotolsma
Copy link
Member Author

I got inspiration from Woo Subscriptions:

I don't know if this is the best inspiration. It is a bit of confusing link. I already created an issue for that: https://github.com/pronamic/pronamic-pay/issues/97. We are now introducing the same issue here 😉.

Not entirely, in the current implementation the links are provided with descriptions. But I was aware of this, which is why I came up with an alternative.

The primary task of the element is "Sharing the action links" with the customer.

This is not the case, I have used the current links regularly. I won't use copying the links much myself. But seeing what the pages look like and what I share with the customer is something I am more likely to do. I also find it difficult to imagine that there are administrators who blindly copy this link and pass it on to their customer, you want to know what you are sharing with a customer. In that respect, it seems to me that the 'View page' is the primary function, and the copy link functionality is secondary.

@rvdsteege
Copy link
Member

FWIW, I think we should go with this… both 'view' and 'copy' can be used this way and satisfies all use cases:

image

@remcotolsma
Copy link
Member Author

FWIW, I think we should go with this… both 'view' and 'copy' can be used this way and satisfies all use cases:

image

Originally posted by @rvdsteege in #180 (comment)

I had this worked out, but this display also requires an additional header. Otherwise users does not know for what the URL is, so for now I will first merge #180 (comment). It is not perfect, but we also have ideas to revise these 3 pages/links:

@remcotolsma remcotolsma merged commit 964b4df into main Sep 4, 2024
19 of 20 checks passed
@remcotolsma remcotolsma deleted the subscription-action-links-ui branch September 4, 2024 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants