Certificate social media / URL sharing #2524
Conversation
| <Page> | ||
| <SharePopover | ||
| open={shareOpen} | ||
| title={`${title} Certificate - MIT Open Learning`} |
There was a problem hiding this comment.
close enough, but I like this a bit better:
| title={`${title} Certificate - MIT Open Learning`} | |
| title={`${title} Certificate issued by MIT Open Learning`} |
|
|
||
| return standardizeMetadata({ | ||
| title: `${userName}'s ${displayType}`, | ||
| description: `${userName} has successfully completed the Universal Artificial Intelligence ${displayType}: ${title}`, |
There was a problem hiding this comment.
Can you give me an example of how this renders?
We have to be careful about hard-coding "Universal Artificial Intelligence" because this won't stay forever. In a case like this, which isn't very visible, I would just drop "Universal Artificial Intelligence" altogether for better future-compatibility.
| description: `${userName} has successfully completed the Universal Artificial Intelligence ${displayType}: ${title}`, | |
| description: `${userName} has successfully completed the ${displayType}: ${title}`, |
There was a problem hiding this comment.
That currently looks like: "Joe Learner has successfully completed the Universal Artificial Intelligence Module Certificate: The Course Name".
With your suggested change: "Joe Learner has successfully completed the Module Certificate: The Course Name".
displayType is either "Module Certificate", "Series Certificate" or "MicroMasters® Certificate".
We do have "Universal Artificial Intelligence" hardcoded on the certificate text itself ("has successfully completed all requirements of the Universal Artificial Intelligence module").
There was a problem hiding this comment.
This is working well! I actually got the clipboard copying to work locally on localhost:8062 ... Sometimes localhost doesn't work because of CORS, etc, but the browser doesn't actually need to make any requests with the prefetching.
A simple requested change... we should add some aria-labels to the social links.
Additionally, it would be good to add some tests for sharepopover, either a dedicated test file or in the certificate page test.
It would be nice if jsx-a11y linter caught the a11y issue above more reliably, but in lieu of that, I find these issues are usually pretty readily discovered when writing tests with getByRole, etc.
| onClick={(event) => { | ||
| const input = event.currentTarget.querySelector("input") | ||
| if (!input) return | ||
| input.select() |
There was a problem hiding this comment.
nice touch :)
| edge="circular" | ||
| variant="bordered" | ||
| startIcon={<RedLinkIcon />} | ||
| aria-label={copyText} |
There was a problem hiding this comment.
Shouldn't need an explicit label, will use contents.
| const facebookLink = document.querySelector(`a[href="${facebookHref}"]`) | ||
| const twitterLink = document.querySelector(`a[href="${twitterHref}"]`) | ||
| const linkedinLink = document.querySelector(`a[href="${linkedinHref}"]`) |
There was a problem hiding this comment.
I think I failed to put this comment on a specific line or forgot to press submit, but :
we should add some aria-labels to the social links.
The social links right now just have icons w/o any accessible label.
Suggestion: Always try using getByRole(role, { name }) for selections like this. If it doesn't work / works awkwardly, it means we are missing some aria labels.
So:
- Add aria labels to the social links
- then
getByRole("link", { name: "Share on Facebook"})etc
| expect(facebookLink).toBeInTheDocument() | ||
| expect(twitterLink).toBeInTheDocument() | ||
| expect(linkedinLink).toBeInTheDocument() |
There was a problem hiding this comment.
Once you use getByRole, this will be redundant (it checks for in do + visible + not disabled, etc).
| const socialLinks = document.querySelectorAll("a") | ||
| socialLinks.forEach((link) => { | ||
| expect(link).toHaveAttribute("target", "_blank") | ||
| }) |
There was a problem hiding this comment.
- Request: use
getAllByRoleLink("link") - Suggest adding an assertion about the expected number of links
- _Note: As currently written, this would pass if there were 0 links (though I'm sure some other test would fail). With
getAllByRole, it will fail if there are 0 links, requires at least 1+. Still worth adding an expected number of links, imo.
- _Note: As currently written, this would pass if there were 0 links (though I'm sure some other test would fail). With
| const copyButton = screen.getByText("Copy Link") | ||
| fireEvent.click(copyButton) |
There was a problem hiding this comment.
| const copyButton = screen.getByText("Copy Link") | |
| fireEvent.click(copyButton) | |
| const copyButton = screen.getByRole("button", { name: "Copy Link" } ) | |
| user.click(copyButton) // from @testing-library/user-events |
https://testing-library.com/docs/user-event/intro/#differences-from-fireevent
frontends/main/src/app-pages/CertificatePage/CertificatePage.test.tsx
Outdated
Show resolved
Hide resolved
…est.tsx Co-authored-by: Chris Chudzicki <[email protected]>
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/8616
Description (What does it do?)
Adds a share button and popover from the designs in Figma.
Adds Open Graph metadata for social media previews:
Screenshots (if appropriate):
How can this be tested?
Hopefully you have mitxonline running locally with a certificate set up (otherwise please message me for a run through).
navigator.clipboardis only available in secure contexts, not HTTP, so locally this fails silently.