-
Notifications
You must be signed in to change notification settings - Fork 95
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
SKOS XL popup on concept page #1718
base: main
Are you sure you want to change the base?
Conversation
Looks good and works somewhat as expected. With mouse, the popup becomes visible with a mouse over, and it becomes hidden with a mouse out. The popup can locked in place by clicking the button so that it remains visible after mouse out. I'd expect like to see the popup becoming hidden with another click on the button. However, it seem this cannot be done with pure CSS buttons. With touch screen (my laptop), the current functionality is: click on the button and the popup becomes visible, click on the button again and nothging happens, but click away and the popup becomes hidden. My suggestions:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1718 +/- ##
=========================================
Coverage 70.91% 70.91%
Complexity 1651 1651
=========================================
Files 33 33
Lines 4332 4332
=========================================
Hits 3072 3072
Misses 1260 1260 ☔ View full report in Codecov by Sentry. |
…sier maintenance and faster test cycles
I added the remaining two cases of SKOS XL popups as well as Cypress tests for them. See the updated description in the OP. Now ready for review. Accessibility could probably be improved further, ideas welcome! |
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.
Looks pretty good! I could think of some clear improvements to accessibility, as requested.
Also a small addition to the cypress tests (example included in the review comments).
Quality Gate failedFailed conditions |
I implemented the proposed changes (generating the id's was a bit painful!). Also, I made the clickable area around the info icons a bit larger to make it easier to hit them e.g. on touchscreen devices. |
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.
LGTM! Everything looks fine in WAVE, too. I added a very minor fix to CSS.
|
||
.tooltip-html button:focus { | ||
border: 2px solid var(--vocab-text); | ||
border-radius: 0; |
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 would add a compensation for the border in the margin like so:
border-radius: 0; | |
border-radius: 0; | |
margin: -4px; |
So that the text doesn't jitter when the button is pushed (Chromium, not sure how it would be with other browsers).
Reasons for creating this PR
See #1714. This PR implements a popup display for SKOS XL information for
The popup is implemented in pure CSS. There are also Cypress tests to verify that the popup is triggered when clicking the icons for all three cases.
The Cypress test file
concept.cy.js
was starting to become too unwieldy, so I split off some of the tests into a separateconcept-full-vs-partial.cy.js
file. The PR also contains a minor typo fix for the concept page template.Link to relevant issue(s), if any
Description of the changes in this PR
Here are a couple of examples showing what this looks like:
Known problems or uncertainties in this PR
Checklist
.sr-only
class, color contrast)