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

pkp/pkp-lib#10319 Implement ORCID brand guidelines #10346

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

taslangraham
Copy link
Contributor

For #10319

@taslangraham taslangraham force-pushed the i10319-main branch 4 times, most recently from be5444a to c0fc1a3 Compare September 4, 2024 18:07
@taslangraham taslangraham marked this pull request as ready for review September 4, 2024 19:03
"https://info.orcid.org/wp-content/uploads/2020/12/ORCIDiD_icon16x16.png\" "
"width='16' height='16' alt=\"ORCID iD icon\" style=\"display: block; margin: "
"0 .5em 0 0; padding: 0; float: left;\"/>Register or Connect your ORCID "
"iD</a><br/>\n"
"<br>\n"
"<br>\n"
"If the button above does not work, please use this link to register or connect your ORCID iD: <a href=\"{$authorOrcidUrl}\">{$authorOrcidUrl}.</a>\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Devika008 this is for the ORCID invitation where there's a button and the URL in plain text. Just wanted to check with you on the wording here. I know some email services will completely remove the button so not sure what best practices is here. Thanks!

Copy link

@Devika008 Devika008 Sep 20, 2024

Choose a reason for hiding this comment

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

Even if email services remove buttons, we can still highlight the link in three simple ways using clear text links.

  1. Use Descriptive Text Links: Click here to review your submission: www.example.com/review
  2. Highlight the Links: Access your dashboard here: www.example.com/dashboard
  3. To complete your submission, follow this link: www.example.com/complete

I think option 2 is something which is straight and to the point

Click here to verify your account with ORCID: {link}

{include file="form/orcidProfile.tpl"}
{if $orcid && $orcidAuthenticated}
{include file="linkAction/buttonConfirmationLinkAction.tpl" titleIcon="modal_delete" buttonSelector="#deleteOrcidButton" dialogText="orcid.field.deleteOrcidModal.message"}
{* {fbvElement type="button" label="common.delete" class="pkp_button pkp_button_offset"}*}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this commented-out fbvElement be removed completely?


{if $verifySuccess}
<script type="text/javascript">
setTimeout(function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be spaces instead of tabs here and the next two lines.

@@ -36,6 +36,9 @@
{elseif $submissionNotPublished}
{translate key="orcid.verify.sendSubmissionToOrcid.notpublished"}
{/if}
<span class='orcid-redirect'>
{translate key="orcid.verify.success.redirect" contextName=$contextName}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this redirect part of the recommendations per ORCID? At first I thought something was wrong because I didn't have time to read the message saying it would automatically be redirected. If the redirect is desired, I think the notification about it needs to be more front-and-centre and maybe a little longer given the amount of text to process. Curious what you think, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of the recommendations in the ORCID docs. You can see it under the "Provide feedback that their record has been connected" section.
We could make the redirect happen after about 10 seconds(it currently happens after 5) and make the text bold.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perfect, that sounds good to me. Maybe try having the redirect notice on a new line as well to see how that looks?

@@ -51,16 +51,24 @@
{/fbvFormSection}

{if $orcidEnabled}

<div class="orcid_container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if this broke here specifically, but the code in AuthorizeUserData that's supposed to place the ORCID value onto the page and remove the button isn't working anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. The issue was actually here. It was still attempting to refresh the "public"(index 3) tab in the user profile. I think it's a regression from this PR that moved the ORCID stuff from the "public" tab to "identify"

@@ -518,15 +518,21 @@ msgstr ""
"submission to your ORCID profile on publication.<br>\n"
"Visit the link to the official ORCID website, login with your profile and "
"authorize the access by following the instructions.<br>\n"
"<a href=\"{$authorOrcidUrl}\"><img id=\"orcid-id-logo\" src=\""
"<br>\n"
"<a href=\"{$authorOrcidUrl}\" style=\"display: inline-flex; align-items: center; background-color: white; text-align: center; padding: 10px 20px; text-decoration: none; border-radius: 5px; border: 2px solid #d7d4d4;\">"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks okay to me, but just want to run it by @Devika008 since (to my knowledge) this is the first "button" we've included in emails. Just want to make sure it's following best practices for compatibility and accessibility, especially given the varying degrees of CSS support in email clients.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've done a bit more looking into things and this should be okay.

@taslangraham taslangraham force-pushed the i10319-main branch 2 times, most recently from f574d78 to 733b488 Compare September 11, 2024 18:53
@taslangraham
Copy link
Contributor Author

taslangraham commented Sep 11, 2024

@ewhanson Thanks for the review. I've made the suggested changes. Awaiting @Devika008's input on your questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants