-
Notifications
You must be signed in to change notification settings - Fork 96
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
Copy to clipboard functionality for concept page #1590
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1590 +/- ##
=========================================
Coverage 70.54% 70.54%
Complexity 1644 1644
=========================================
Files 32 32
Lines 4315 4315
=========================================
Hits 3044 3044
Misses 1271 1271 ☔ View full report in Codecov by Sentry. |
8f56ee5
to
c93eaec
Compare
Rebased on current |
TODO:
|
972b16a
to
44e1a93
Compare
SonarCloud complains about duplicated code in Cypress tests. Well, duh, tests are repetitive... |
// register the copyToClipboard function as event an handler for the copy buttons | ||
registerCopyToClipboardEvents() | ||
|
||
// re-register the event handlers after partial page loads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line fills me with dread. We should make sure all event handlers are re-registered collectively. Partial page loading as a specific test case hasn't been used as of yet. Should we have an issue about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a more general issue, but currently we don't have that many event handlers and AFAICT only the concept-mappings component needs to re-register its event handler after a partial page load, and it already does that.
// check that the clipboard now contains "music research" | ||
// NOTE: This test may fail when running Cypress interactively in a browser. | ||
// The reason is browser security policies for accessing the clipboard. | ||
// If that happens, make sure the browser window has focus and re-run the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The things you learn..! -__-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😁 Ideally the test would just be skipped when this happens. Not sure if that's easy to implement.
I would really want to see a hightlight for the copy button when used by keyboard. Test case:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keyboard navigation needs a highlight hint, otherwise it looks good
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, and it works for me - we can ask for a graphic designer's opinion later.
Reasons for creating this PR
Implement copy to clipboard functionality, which is one of the required features for the concept page (#1484).
Link to relevant issue(s), if any
Description of the changes in this PR
Known problems or uncertainties in this PR
npx cypress open
) if the Cypress window isn't focused (for example when working in an IDE) due to browser security policy; they always work when run headless (npx cypress run
) and under CIChecklist
.sr-only
class, color contrast)