Skip to content

UX Improvements to shareable certificates#1387

Merged
ideadude merged 13 commits intogocodebox:devfrom
imknight:bug-fix-1379
Jun 26, 2024
Merged

UX Improvements to shareable certificates#1387
ideadude merged 13 commits intogocodebox:devfrom
imknight:bug-fix-1379

Conversation

@imknight
Copy link
Copy Markdown
Contributor

@imknight imknight commented Oct 16, 2020

Description

Fixes #1379

How has this been tested?

Screenshots

Captura de pantalla 2024-06-04 a las 11 15 40 a  m ld cause existing functionality to not work as expected) -->

Checklist:

  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

<?php endif; ?>

<?php if ( $is_sharing_enabled ) : ?>
<span class="llms-button-secondary">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was envisioning a read-only text input in favor of a button that doesn't do anything when clicked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi, I thought the proposal mention "When sharing is enabled, a read-only text input should be output with the certificate's URL populated in it. "?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi @imknight ,
yes so in place of this <span> that looks like a button, @thomasplevy was envisioning a read-only text input, as mentioned in the proposal.
Is that anything I'm missing?!

Copy link
Copy Markdown
Contributor Author

@imknight imknight Oct 19, 2020

Choose a reason for hiding this comment

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

oh got it, finally, I understand, will update the code.
I can change it into input easily, but need some tweak on styles, shall it combine within this change request?
can't really get the gulp working, any documentation that I can read about workflow to tweak the style?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have you had a look at our Installing for Development Docs: https://github.com/gocodebox/lifterlms/blob/trunk/docs/installing.md

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The CSS is probably less important than making sure this fundamentally works. Although now that I'm looking at this again we're going to need to add some UX to the field too...

I'm sure anyone who sees a field is also going to expect the following:

  1. When you click the field it should highlight everything
  2. There should be a "Copy" button next to the field

So we'll need to add some JS to this to power the UX

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I got it for 2 so that if a user clicks on the copy button, it will copy the permalink to the clipboard, but what is 1 about? highlight everything?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of having to double click (or single click and then ctrl/cmd + a) to highlight the whole link, it would be simpler for the end user for them to click once in the field and have the entire contents of the field automatically selected/highlighted.

Here's a quick gif of how google drive handles this (you maybe can't tell but I'm just clicking the field and it automatically highlights the whole contents)

Peek 2020-10-20 09-25

I find that most apps that let you copy a link have this behavior automatically so if we don't do it we'll get support requests about how it doesn't work right, almost guaranteed.

Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, just to test the concept, it can be done pretty simply with some JS like this:

document.getELementById( 'test-input' ).addEventListener( 'focus', e => {
     e.target.select();
} );

@thomasplevy thomasplevy changed the title Bug fix 1379 UX Improvements to shareable certificates Oct 20, 2020
@thomasplevy thomasplevy removed the request for review from eri-trabiccolo October 22, 2020 17:25
Copy link
Copy Markdown
Contributor

@thomasplevy thomasplevy left a comment

Choose a reason for hiding this comment

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

Sorry @imknight and thanks for your patience on this one.

I'm not a huge fan of doing an inline script like this on a client-facing page. We make concessions sometimes but this doesn't feel like a good place to do it.

Especially because on a closer inspection I think we can do it with a oneliner on the input itself...

onfocus="this.select()" I think does it, right?

Then we don't have to have an inline <script> tag and we don't have to add a JS asset specifically for this purpose.

Script event handler attributes aren't that attractive and I also like to avoid them but in this case I think there's a good argument for using them.

What do you think?

And, again, thanks for your patience!

@imknight
Copy link
Copy Markdown
Contributor Author

ok, will try to work on oneliner script, I thought for a template is good to use the inline script.

@thomasplevy thomasplevy added this to the Bug Scrub November 2020 milestone Nov 3, 2020
Copy link
Copy Markdown
Contributor

@thomasplevy thomasplevy left a comment

Choose a reason for hiding this comment

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

I've been holding on this because now that the initial work is complete I think additional UI work is needed before this can be shipped.

  1. A copy button is still needed. This adds some complexity due to the nature of copying to the clipboard via browser interactions. It will require a JS file to be included on the certificate to power the UI.
  2. Looking at line of buttons makes me realize that we need to change our strategy on how this content is output below the certificate. I don't have a good solution in mind so I don't know how to push this forward. What I'm seeing today doesn't look right with the input (and then later a new button from 1 above) but I don't have any alternatives in mind.

Thank for your work thus far @imknight, we'll get this merged in at some point but at this moment this isn't a high-enough priority item for us to take over internally and since I don't know exactly what direction to move in I don't know how to properly relay this to you.

I need to spend some design time here and I don't have that capacity right now.

Thanks,

@imknight
Copy link
Copy Markdown
Contributor Author

imknight commented Nov 7, 2020

noted and thank

@thomasplevy thomasplevy added the hacktoberfest PRs for this issue count towards Hacktoberfest contributions! label Nov 30, 2020
@eri-trabiccolo eri-trabiccolo marked this pull request as draft February 3, 2023 15:26
Co-authored-by: Thomas Patrick Levy <thomasplevy@gmail.com>
@brianhogg
Copy link
Copy Markdown
Contributor

We may want to add a "Copy to Clipboard" link/button (see Advanced Coupons UI for accessible suggestions on how to do this), but otherwise making it clear that they need to share the current URL looks good.

Test to make sure UI looks reasonable, potentially add copy to clipboard, then merge.

@brianhogg
Copy link
Copy Markdown
Contributor

Added screenshot. Awaiting approval or if we want to re-work the design a bit.

@brianhogg brianhogg changed the base branch from trunk to dev June 3, 2024 20:30
@brianhogg brianhogg marked this pull request as ready for review June 4, 2024 15:21
@brianhogg brianhogg requested a review from ideadude as a code owner June 4, 2024 15:21
@brianhogg
Copy link
Copy Markdown
Contributor

After discussion, switched text input to a button that, when clicked, copies the URL to the clipboard.

@imknight
Copy link
Copy Markdown
Contributor Author

lol from 2020 until now, finally see it merge!

@ideadude ideadude merged commit daa5227 into gocodebox:dev Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest PRs for this issue count towards Hacktoberfest contributions!

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Certificates: New sharing button creates confusing user experience when button is next to the "Save" button

5 participants