Added accessible links SCSS #5
Conversation
Will show blue or purple links for visited/unvisted content.
abias
left a comment
abias
left a comment
There was a problem hiding this comment.
Hi @pd431,
many thanks for submitting this contribution, this is highly appreciated!
I have just added several comments to the file with improvement requests.
Additionally, you will find the linter results on https://github.com/moodle-an-hochschulen/moodle-theme_boost_union_snippets/pull/5/checks
I hope that you will be able to resolve these glitches in reasonable time. Afterwards, the snippet is ready to land.
Please bear with us with these change requests. We are just setting up the contribution processes for this repo and will provide better contribution documentation as soon as we have learnt what contributors need.
Cheers,
Alex
| * Goal: accessibility | ||
| * Description: Makes most links across the site blue and underlined, and purple if visited. Includes activity links, but excludes section titles, buttons and other link controls across the site | ||
| * Creator: Pierre Camaly de Brosses | ||
| * Tracker issue: <snippet tracker issue> |
There was a problem hiding this comment.
You can omit this line if your snippet is not targetted to any Moodle core tracker issue.
| * Creator: Pierre Camaly de Brosses | ||
| * Tracker issue: <snippet tracker issue> | ||
| * Tested on: Moodle 4.5, Firefox, Chrome and Safari | ||
| * Usage note: None |
There was a problem hiding this comment.
You can omit this line if there isn't any special to consider when using this snippet
| * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later | ||
| */ | ||
|
|
||
| @mixin UI-elements { |
There was a problem hiding this comment.
Building the SCSS snippet with a mixin is generally fine, but please use a more unique mixing name with all lowercase characters. The reason is that the SCSS snippet will be added to the full SCSS stack of Boost Union and we want to avoid any name clashes.
Something like @mixin busnippet-accessiblelinks-uielements should be fine.
| @@ -0,0 +1,49 @@ | |||
| /** | |||
| * Snippet Title: Accessible Link Colors | |||
| * Scope: global | |||
There was a problem hiding this comment.
Your snippet uses the .pagelayout-incourse, .pagelayout-course selectors. But here you are saying Scope: global and in the description you are saying across the site.
I am wondering what intention really is. If this snippet should have a global scope, then the CSS selectors are wrong. If this snippet should have a course scope, then the comments here should be fixed.
I would be fine with both.
There was a problem hiding this comment.
I wrote that code last year and forgot the logic I'd put in it. Course scope is definitely correct. I've been describing it as "across the site" internally, as that makes most sense to colleagues, so did this out of habit without thinking.
There was a problem hiding this comment.
Up to now, the snippet files are named with snake case.
If you could change the file names to accessible_links.*, that would be highly appreciated.
There was a problem hiding this comment.
Could you create a more clearer screenshot?
If you shrink the width of the browser window to reduce whitespace and create the screenshot with original resolution, it should be better to understand what the snippet will change.
Conformed name to snake case, adjusted scope to course only, clarified description adopted more unique mixin link cleaned up indentation and spaces
moved from 2 spaces to 4
removed indentation in the mixin
|
Apologies for the sucessive commits, I have no experience in writing linted code. |
2 participants
We've had the need to enforce traditional link colors separately from the main brand color as defined by boostunion.
This snippet excludes most UI elements, like headers, buttons and dropdowns, and colors the rest. Will show blue or purple links for visited/unvisted content.
Does not apply inside the text editors, as they're loaded as iframes and don't pull in the full boost-union SCSS.