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

Theme: Fixed header FIP image's alt text. #251

Merged

Conversation

EricDunsworth
Copy link
Member

@EricDunsworth EricDunsworth commented May 12, 2017

A bilingual FIP image is situated in the top-right of all of the usability theme's pages. Until now, in all standard/server message/splash page templates, the image's alt text was unilingual and based on the current page language. However, that wasn't an accurate representation of the text in the image.

This commit adds extra handlebars conditions to make the FIP's alt text bilingual depending on the current page language. In French pages, it'll be represented as French/English bilingual text. In all other pages, it'll be represented as English/French bilingual text. Furthermore, in multilingual pages, a lang="en" attribute will be added to the image's object element in order to assist screen readers.

Related to wet-boew/GCWeb#1241.

@EricDunsworth EricDunsworth changed the title Theme: Fixed header FIP image's incomplete alt text. [Do not merge] Theme: Fixed header FIP image's incomplete alt text. May 15, 2017
@EricDunsworth
Copy link
Member Author

I've just noticed that in multilingual pages, as things stand, the FIP's alt text is based on the current page's language (not the text that's in the image itself). Since this PR and wet-boew/theme-gc-intranet#171 didn't account for that in their current forms, they'll need to be revised.

@EricDunsworth EricDunsworth changed the title [Do not merge] Theme: Fixed header FIP image's incomplete alt text. Theme: Fixed header FIP image's alt text. Oct 17, 2017
@EricDunsworth EricDunsworth force-pushed the v4.0-theme-fip-alt-bilingual branch 2 times, most recently from 4094b81 to e4169f9 Compare October 17, 2017 14:36
{
"officiallanguage": "<%= language === 'en' || language === 'fr' %>"
}
---
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that the line break just after this spot will be retained in generated HTML code. So in generated server message page templates, the file's first line will be empty, followed by the HTML doctype on the second line. It'll still pass HTML validation, but looks kind of odd.

Btw this issue seems to affect all handlebars files that contain YAML front matter (YFM). The line break after the second instance of --- is always included in generated HTML code. It's only really obvious in this case since it precedes the page's doctype.

@nschonni @LaurentGoderre Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nschonni @LaurentGoderre Any thoughts regarding my previous line comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A leading line can also blow up old IE i believe. Or at least it automagically triggers compat mode

Copy link
Member Author

@EricDunsworth EricDunsworth Nov 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nschonni According to https://wet-boew.github.io/v4.0-ci/docs/versions/rdmp-en.html, WET 4.0 technically no longer supports IE9 and below. So that might not be as bad in practice as it could've been. Although I'd still prefer avoiding the line break.

All I can think of to easily avoid the line break would be to move <!DOCTYPE html> to the end of line 5 (which would place it right beside ---). Only drawback to it is that it'd look a bit strange to anyone that looks through this HBS file.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna go ahead and place --- and <!DOCTYPE html> side-by-side on the same line to omit the leading line break. Can't think of a more elegant way of getting rid of it atm.

@EricDunsworth
Copy link
Member Author

EricDunsworth commented Oct 17, 2017

Btw this PR is currently running into a Travis-CI error that isn't related to any of its changes.

Travis-CI error (excerpt):

Running "sasslint:all" (sasslint) task

src/_variables.scss

54:3 error Property image appears to be spelled incorrectly no-misspelled-properties

✖ 1 problem (1 error, 0 warnings)

Warning: � Use --force to continue.

Aborted due to warnings.

Related file:
src/_variables.scss (line 54)

image isn't misspelt on that line and it looks like a proper usage of nested properties to me. Maybe the linter is trying to evaluate image as if it was a standalone CSS property (without realizing it's supposed to be an extension of background: {...} to form background-image)?

@nschonni @LaurentGoderre Thoughts?

@nschonni
Copy link
Member

I would probably try to pull the conditional out of the nested property. EX: use the conditional to set variable, then use the nested property. In this case though, i'm not sure the nested property really helps readablitiy

@EricDunsworth
Copy link
Member Author

@nschonni I can give it a shot and send over a separate PR for it (since it has nothing to do with this PR's changes).

But having said that, isn't the root cause of this issue a bug in the SCSS linter? I agree that the current SCSS structure at that spot does look a bit odd, but IMO it's technically valid. I wouldn't of expected it to trigger linting errors.

@nschonni
Copy link
Member

@EricDunsworth might be this sasstools/sass-lint#1112

@EricDunsworth
Copy link
Member Author

EricDunsworth commented Oct 18, 2017

@nschonni Thanks for pointing that out :)!

From what I can tell, the fix for that (sasstools/sass-lint#1113) is included in sass-lint 1.12.0+, but wasn't mentioned in its release notes. The failed Travis-CI build was using grunt-sass-lint 0.2.4, which is in turn using sass-lint ^1.12.0.

Maybe the changes that caused sasstools/sass-lint#1112 also caused problems with nested properties that are preceded by conditions?

@EricDunsworth
Copy link
Member Author

@nschonni @LaurentGoderre FYI I've just opened sasstools/sass-lint#1158.

A bilingual FIP image is situated in the top-right of all of the usability theme's pages. Until now, in all standard/server message/splash page templates, the image's alt text was unilingual and based on the current page language. However, that wasn't an accurate representation of the text in the image.

This commit adds extra handlebars conditions to make the FIP's alt text bilingual depending on the current page language. In French pages, it'll be represented as French/English bilingual text. In all other pages, it'll be represented as English/French bilingual text. Furthermore, in multilingual pages, a lang="en" attribute will be added to the image's object element in order to assist screen readers.

Related to wet-boew/GCWeb#1241.
@LaurentGoderre LaurentGoderre merged commit 622cf93 into wet-boew:master Nov 22, 2017
EricDunsworth added a commit to EricDunsworth/theme-gc-intranet that referenced this pull request Nov 22, 2017
One of wet-boew#171's changes introduced a line break before the HTML doctype in all unminified server message page templates. wet-boew/theme-gcwu-fegc#251's discussion indicated that may potentially prevent HTML standards mode from activating in certain browsers. Plus it looks strange for generated HTML pages to not immediately start off with a doctype.

This commit removes the line break by placing the YFM block's 3 closing dashes and the HTML doctype on the same line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants