fix: UIO focus styling (resolves #231) #233

TeddTech · 2020-05-11T20:50:34Z

This pull request has been linted by running npm run lint without errors
This pull request has been tested by running npm run start and reviewing affected routes
This pull request has been built by running npm run build without errors
This isn't a duplicate of an existing pull request

Description

This PR adds focus and hover styling to uio contrast options.

Steps to test

run npm run start
select a uio contrast option using the uio tab in the top right corner of webpage
focus and hover over different elements on the webpage
repeat step 3 for the other webpages on website

Expected behavior:
Clear and appropriate focus/hover styling should appear on all webpages when uio contrast options are implemented.

Additional information

N/A

Related issues

closes #231

TeddTech · 2020-05-11T20:51:46Z

This PR needs to be merged AFTER PR #230

TeddTech · 2020-05-11T20:56:30Z

src/scss/abstracts/_mixins.scss

+	a:focus,
+	a:hover,
+	#defaultContainer header .site-nav-wrapper .site-nav nav a:focus,
+	#defaultContainer header .site-nav-wrapper .site-nav nav a:hover,
+	#defaultContainer .content-wrapper .tags-info .tags a:focus,
+	#defaultContainer .content-wrapper .tags-info .tags a:hover, {
+		border: rem(2) solid $c;
+	}


@greatislander I came up with this solution to get by the linting error that was appearing during check-in. Let me know what you think

Awesome, that was what I was going to suggest!

TeddTech · 2020-06-04T01:49:17Z

There is an outstanding error in this PR that I feel should be addressed in a separate issue.

cindyli · 2020-06-04T14:32:06Z

src/scss/vendors/_vendors.scss

+	[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) {
+		// Search icon; IDRC, OCADU and Canada logos
+		header .site-nav-wrapper .site-nav .search-container svg {
+			height: 1.5rem;


The circle around the search icon on the high contrast is still not a perfect round:

Reducing the size of the svg seems working better. These are values I tried:

hight: 1.2rem; margin: -1.6rem; width: 1.2rem;

Please do use rem() function in scss files rather than the straight rem unit. Thanks.

cindyli · 2020-06-04T14:43:31Z

src/scss/abstracts/_mixins.scss

+		box-shadow: inset 0 0 0 rem(2) $fColor;
+	}
+
+	#defaultContainer .content-wrapper main .pagination .pagination-list li a[aria-current="page"] {


I feel it might be better to combine uio-focus-style() and adapt-to-highContrast() functions into one and name it adapt-to-contrastThemes() because:

Some code in uio-focus-style() deals with non-focus stylings such as this section while some code in adapt-to-highContrast() deals with focus stylings such as line 41 but not in uio-focus-style() function;

It's hard to separate out focus/hover code when it is part of an element handling. The styles for social media icons at line 35 is an example.

Please add code comments to each section in the combined function to explain what's handled by that section. Thanks.

cindyli · 2020-06-04T14:51:22Z

src/scss/abstracts/_mixins.scss

+	button:hover,
+	li .pagination-link,
+	li .pagination-link::before {
+		box-shadow: inset 0 0 0 rem(2) $fColor;


Replacing $fColor with currentColor works the same way. This change will help to move this section of code out of the function into the more generic handling within [class^="gpii-ext-theme"], [class^="fl-theme"]:not(.fl-theme-prefsEditor-default). This move will help to reduce the size of the generated css.

cindyli · 2020-06-04T14:55:05Z

src/scss/vendors/_vendors.scss

+			background-image: none;
+		}
+
+		footer .funders .footer-logos a {


.footer-logos is not a child of .funders. This css doesn't apply to logo svgs on the footer. The focus/hover styling on these logos are broken now.

cindyli · 2020-06-04T15:02:29Z

src/scss/abstracts/_mixins.scss

+		color: $bColor !important; // Enactors.css in infusion uses !important on this element
+	}
+
+	#defaultContainer .content-wrapper main .pagination .pagination-list li a[aria-current="page"]:hover,


This focus/hover styling is for the same element above at line 19. Can you move it in there in the format of:

#defaultContainer .content-wrapper main .pagination .pagination-list li a[aria-current="page"] { // regular styles &:hover, &:focus { // focus/hover styles }

Thanks.

TeddTech · 2020-06-04T19:48:58Z

src/scss/vendors/_vendors.scss

-[class^="gpii-ext-theme"],
+
+// TODO: Address style lint errors when line below is uncommented
+// [class^="gpii-ext-theme"],


When including the selector .gpii-ext-theme-#{$theme} for UIO + themes the following linting errors occur:

✖ stylelint found some errors. Please fix them and try committing again. src/scss/vendors/_vendors.scss 20:3 ✖ Expected selector "[class^="gpii-ext-theme"] footer .footer-logos a" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) footer .funders a" no-descending-specificity 27:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer a:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer a:focus" no-descending-specificity 29:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer header .site-nav-wrapper .site-nav nav a:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer header .site-nav-wrapper .site-nav nav a:focus" no-descending-specificity 30:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer .content-wrapper .tags-info .tags a:focus" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer header .site-nav-wrapper .site-nav nav a:focus" no-descending-specificity 30:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer .content-wrapper .tags-info .tags a:focus" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer header .site-nav-wrapper .site-nav nav a:hover" no-descending-specificity 31:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer .content-wrapper .tags-info .tags a:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer header .site-nav-wrapper .site-nav nav a:focus" no-descending-specificity 31:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer .content-wrapper .tags-info .tags a:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer header .site-nav-wrapper .site-nav nav a:hover" no-descending-specificity 31:2 ✖ Expected selector "[class^="gpii-ext-theme"] #defaultContainer .content-wrapper .tags-info .tags a:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) #defaultContainer .content-wrapper .tags-info .tags a:focus" no-descending-specificity 34:2 ✖ Expected selector "[class^="gpii-ext-theme"] .card:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) .card:focus" no-descending-specificity 36:2 ✖ Expected selector "[class^="gpii-ext-theme"] button:hover" to come before selector "[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) button:focus"

While working on #266, I found CSS attached with the generic selector [class^="gpii-ext-theme"], [class^="fl-theme"]:not(.fl-theme-prefsEditor-default) sometimes are not applied when another UIO style that is attached with a specific theme selector kicks in. The solution (#269) is to hook up CSS with each specific theme selector.

From the lesson learnt there, I feel the same rule applies to other CSS you have here too. It means to move all css inside [class^="gpii-ext-theme"], [class^="fl-theme"]:not(.fl-theme-prefsEditor-default) to the mixin function. This change will fix this linting error too.

@cindyli I have moved only the selectors with a currentColor value to _mixins.scss file. Let me now if this is a valid a solution for this linting problem and I will request another review

I tested with your update. Unfortunately, the similar issue as #266 appears with other css that are not moved from the generic theme into specific theme selector.

Using hiding background image of home page cards as an example:

Turn on "enhance inputs" -> switch to another theme -> the background images of cards show up

These images should not be shown on contrast themes.

moved selectors with curretColor value to mixins scss

TeddTech · 2020-06-08T17:08:23Z

@cindyli there are inconsistent hover/focus colours for the last 3 uio contrast themes. Buttons, links, and uio preference panel have different hover/focus colours. I am thinking about addressing this issue in a separate PR. What are your thoughts?

cindyli · 2020-06-08T17:35:30Z

@cindyli there are inconsistent hover/focus colours for the last 3 uio contrast themes. Buttons, links, and uio preference panel have different hover/focus colours. I am thinking about addressing this issue in a separate PR. What are your thoughts?

@TeddTech I agree this could be addressed separately if it is an issue.

From what I learnt from @jobara about the last three low contrast themes is they are designed with different set of colors for links, selected texts etc to match contrast themes on Windows (See @jobara's reply on SJRK pull request).

It feels to me the hover/focus style for links is correct. But the hover/focus style for UIO buttons ("show perferences", "hide preferences" and "reset") is in question.

@jobara and @lliskovoi, what do you think?

cindyli · 2020-06-08T18:23:49Z

src/scss/vendors/_vendors.scss

-		stroke-width: rem(3);
-	}
-}
-
 // For styles that require foreground and background colors from each theme
 @each $theme, $fColor, $bColor in $UIOThemes {


It might be better to move this section to before the line 94 since this style belongs to the generic css outside of any specific media query breakpoint. After the change, the structure of this file will be more structured with the media query breakpoints.

cindyli · 2020-06-08T20:55:53Z

src/scss/vendors/_vendors.scss

+	[class^="gpii-ext-theme"],
+	[class^="fl-theme"]:not(.fl-theme-prefsEditor-default) {
+		// Search icon; IDRC, OCADU and Canada logos
+		header .site-nav-wrapper .site-nav .search-container svg {


I believe this css needs to be moved out of the generic selector into theme specific selectors too. Seems you need another loop on $UIOThemes here.

to prevent hover/focus overlap

TeddTech added 20 commits April 6, 2020 07:03

Merge remote-tracking branch 'upstream/master'

67cc1f8

Merge remote-tracking branch 'upstream/master'

20a7494

Merge remote-tracking branch 'upstream/master'

1b11d50

Merge remote-tracking branch 'upstream/master'

211a5a5

Merge remote-tracking branch 'upstream/master'

8ca4ffa

Merge remote-tracking branch 'upstream/dev' into dev

51e7eab

Merge remote-tracking branch 'upstream/dev' into dev

247387e

Merge remote-tracking branch 'upstream/dev' into dev

a0c2179

Merge remote-tracking branch 'upstream/dev' into dev

fb27e7d

fix: add uio

5eb3c71

Merge remote-tracking branch 'upstream/dev' into dev

725ddbc

Merge branch 'dev' into add-uio

da22299

fix: removed infusion folder at root level

97017cb

fix: infusion npm implementation

5f96329

fix: start figma styling of uio tab

b14d1f0

fix: applied figma styling to uio tap using important

233b7ed

fix: add stylelint workaround

092bfea

fix: uio tab styling specificity

e055cd6

fix: remove uio toc styling

78d1a51

fix: add uio contrast focus styling

3818497

TeddTech self-assigned this May 11, 2020

TeddTech added the enhancement New feature or request label May 11, 2020

TeddTech commented May 11, 2020

View reviewed changes

TeddTech added 6 commits May 11, 2020 14:49

fix: add roboto font on uio tab

e0e1671

fix: use box shadow instead of border for focus and hover

7e2b072

fix: implement cindys uiowecount infusion branch

c846398

fix: used preload value on link tags for performance improvement

a351d48

fix: focus/hover styling to cards and search for uio contrast options

abe2cb9

fix: redo remove text size panel from uio options

50bcad6

TeddTech requested a review from cindyli June 4, 2020 01:49

fix: removed duplicate uio themes variable

92f3ee2

cindyli requested changes Jun 4, 2020

View reviewed changes

cindyli mentioned this pull request Jun 4, 2020

fix: the search container no longer overlaps the nav menu on ipad pro (resolves #264) #268

Merged

4 tasks

TeddTech added 2 commits June 4, 2020 10:28

Merge remote-tracking branch 'upstream/dev' into dev

28f1d59

fix: refractored vendors and mixins scss and added comments

a42b255

TeddTech commented Jun 4, 2020

View reviewed changes

TeddTech added 5 commits June 4, 2020 13:07

fix: removed uneeded comments form mixins

9d5e0b1

Merge remote-tracking branch 'upstream/dev' into dev

5125487

Merge branch 'dev' into uio-focus-styling

05557f0

fix: refactored contrast theme styling

1343c9a

moved selectors with curretColor value to mixins scss

fix: remove uneeded comment

f15442b

fix: properly formated rem functions with negative input

6833bcb

cindyli reviewed Jun 8, 2020

View reviewed changes

TeddTech added 2 commits June 8, 2020 11:28

fix: moved each before media queries

5ad376d

fix: refactored all uio contrast themes stlyling into mixins scss

90eb719

TeddTech requested a review from cindyli June 8, 2020 20:27

cindyli reviewed Jun 8, 2020

View reviewed changes

fix: placed media query for svg positioning inside mixins scss

a94b98d

TeddTech requested a review from cindyli June 8, 2020 21:46

TeddTech and others added 2 commits June 8, 2020 15:36

fix: added padding to uio tab on enhance inputs

88869eb

to prevent hover/focus overlap

fix: css clean up and adjust code comments

9952e7b

cindyli approved these changes Jun 9, 2020

View reviewed changes

cindyli merged commit 6e375bc into inclusive-design:dev Jun 9, 2020

cindyli mentioned this pull request Jun 9, 2020

fix: svgs now respond to theme colors when the enhance inputs is on (resolves #266) #269

Closed

4 tasks

TeddTech deleted the uio-focus-styling branch June 9, 2020 15:21

cindyli mentioned this pull request Jun 9, 2020

SVGs change to default when switching contrast themes while enhance inputs is turned on #266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: UIO focus styling (resolves #231) #233

fix: UIO focus styling (resolves #231) #233

TeddTech commented May 11, 2020 •

edited

Loading

TeddTech commented May 11, 2020

TeddTech May 11, 2020

greatislander May 11, 2020

TeddTech commented Jun 4, 2020

cindyli Jun 4, 2020

cindyli Jun 4, 2020

cindyli Jun 4, 2020

cindyli Jun 4, 2020

cindyli Jun 4, 2020

TeddTech Jun 4, 2020

cindyli Jun 5, 2020

TeddTech Jun 8, 2020

cindyli Jun 8, 2020

TeddTech commented Jun 8, 2020

cindyli commented Jun 8, 2020

cindyli Jun 8, 2020

cindyli Jun 8, 2020

fix: UIO focus styling (resolves #231) #233

fix: UIO focus styling (resolves #231) #233

Conversation

TeddTech commented May 11, 2020 • edited Loading

Description

Steps to test

Additional information

Related issues

TeddTech commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeddTech commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeddTech commented Jun 8, 2020

cindyli commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TeddTech commented May 11, 2020 •

edited

Loading