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

[8.x] Accessibility cookie settings #3140

Closed

Conversation

Andrea-Guevara
Copy link
Contributor

@Andrea-Guevara Andrea-Guevara commented Jun 21, 2024

References

Description

Adding and changing some classes in the global scss to make the cookie settings more accessible.

Instructions for Reviewers

List of changes in this PR:

  • Added the variable "--very-dark-cyan" in _custom_variables.scss.
  • Removed the variable ".cm-btn.cm-btn-success.cm-btn-accept-all" and its styling as it was no longer needed.
  • Styled the "Accept all" button by overwriting the style of the ".klaro .cookie-modal .cm-btn.cm-btn-success.cm-btn-accept-all" variables.
  • Added "text-decoration: underline;" to the anchors.
  • Changed the color of some informational texts, which were previously gray and are now lighter to improve contrast.
  • Changed the color of the "Save" button by overwriting the styling of the variables: .klaro .cookie-modal .cm-btn.cm-btn-info.

Checklist

  • Visit https://sandbox.dspace.org in an Incognito browser
  • Klaro cookie popup should appear. Click on "Customize" to bring up the screen above
  • Expand all sections on that screen.
  • Run an accessibility scanner (WAVE or "axe DevTools) and check that the accessibility test is positive.

@tdonohue tdonohue added accessibility 1 APPROVAL pull request only requires a single approval to merge bug port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release labels Jun 21, 2024
@tdonohue
Copy link
Member

tdonohue commented Aug 8, 2024

NOTE: it's possible this PR may have overlap/conflicts with #3199. We've realized in today's Developers Meeting that migrating our cookie consent form to Orejime might fix (or partially fix) the same accessibility issues as this ticket. This is because Orejime is a fork of Klaro which was created to fix accessibility issues in Klaro.

So, anyone reviewing/testing this PR may want to also compare it to #3199. We need to determine if both PRs are necessary, or if migrating to Orejime will fix the issues we've had with accessibility in Klaro.

@IgorBaptist4 IgorBaptist4 force-pushed the AccessibilityCookieSettings branch from 2586c55 to d0c8f03 Compare September 4, 2024 20:32
@tdonohue
Copy link
Member

Per my prior comment, I've realized we may still want to apply this small PR to 8.x and 7.x to fix accessibility in Klaro for those releases of DSpace. However, it should not be applied to 9.0, as #3199 will migrate us to using Orejime for 9.0

Could you update this PR @Andrea-Guevara to apply to dspace-8_x instead of to main? I think that's the best way forward for this PR, as we cannot apply it to main (since #3199 will be applied on main once it is completed).

@tdonohue tdonohue changed the title Accessibility cookie settings [8.x] Accessibility cookie settings Sep 12, 2024
@tdonohue tdonohue self-requested a review September 12, 2024 15:41
@Andrea-Guevara
Copy link
Contributor Author

Good afternoon @tdonohue! I didn't quite understand what you asked me, or even if you asked me anything haha.

Is it to make the PR from a branch in dspace version 8?

@tdonohue
Copy link
Member

@Andrea-Guevara : I was asking if you could change this PR to be against the dspace-8_x branch, so that we can apply it only to the 8.x releases. If you look at the top of the PR, it's currently submitted for the main branch. That main branch is normally the best place to submit a PR.

But, in the case of this PR, we cannot merge this one into main as it will become obsolete once we merge #3199 into main. Essentially, this PR cannot go into 9.0 as we are removing Klaro (which is what this PR updates).

However, if we could change this PR to be sent to the dspace-8_x branch, then we could still accept it for future 8.x releases (as those will still include Klaro). (The dspace-8_x branch is where we maintain the 8.x codebase.)

If this still doesn't make sense, we can probably apply this directly to dspace-8_x once someone has a chance to test this.

@tdonohue
Copy link
Member

Closing as this is replaced by #3328, as we've decided this should only be merged to 8.x and 7.x.

@tdonohue tdonohue closed this Sep 16, 2024
@tdonohue tdonohue removed the port to dspace-7_x This PR needs to be ported to `dspace-7_x` branch for next bug-fix release label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge accessibility bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color Contrast in cookie settings box is not sufficient
2 participants