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

Fixes for IE 10 and 11 layout issues, minor sass-lint rule change #20896

Merged
merged 3 commits into from
Jan 4, 2019

Conversation

katebee
Copy link
Contributor

@katebee katebee commented Jan 3, 2019

What does this change?

👉 Fix for comments ad being pushed to the right in IE

The ad shifts to the right when scrolling down on comments in internet explorer, this is related to it becoming 'sticky' and how the position settings on the wrapper change.

The fix for this is to set a width on the wrapper, since it seems IE is being finicky and otherwise pushing any absolute positioned elements without a width too far over. Other browsers seem to happily be calculating the width based on the child elements (the ad-slot) of the ad-slot-wapper, but not IE. 😠

👉 Fix for Hosted caption layout issue

This PR allows IE to fallback to a valid CSS setting since IE 10 and 11 do not currently recognise 'unset':

screen shot 2019-01-03 at 15 07 31

👉 Update sass-lint settings to allow left property to be duplicated

This allows left to be duplicated within a block so long as the duplicates are next to one another. We can then lean on the cascade to let browsers use a fallback when facing css they do not understand.

What is the value of this and can you measure success?

BEFORE:

image

❌ Above: comments slot sticky offset right going wild
❌ Below: caption text for the image is overlaying the submeta icons

screen shot 2019-01-03 at 15 01 41

AFTER:

image

✅ Above: intended position of the comments slot
✅ Below: intended position of the caption text

screen shot 2019-01-03 at 15 26 41

The picture above was taken in Chrome. Since Chrome recognises 'unset' this overrides the 'auto'. However, IE will not recognise 'unset' as valid CSS so respects the 'auto' instead... Hooray for cascading style sheets!

Checklist

Does this affect other platforms?

Nope

Does this affect GLabs Paid Content Pages? Should it have support for Paid Content?

Affects Hosted pages @guardian/commercial-dev

Does this change break ad-free?

Nope

Tested

  • Locally
  • On CODE

@katebee katebee requested review from NataliaLKB and a team January 3, 2019 19:04
@PRBuilds
Copy link

PRBuilds commented Jan 3, 2019

PRbuilds results:

Screenshots
wide.pngdesktop.pngtablet.pngmobile.png

💚 Exceptions
thrown-exceptions.js

💚 A11y validation
a11y-report.txt

💚 Microdata Validation
microdata.txt

Apache Benchmark Load Testing
loadtesting.txt

LightHouse Reporting
1546542545.report.html

--automated message

Copy link
Member

@kelvin-chappell kelvin-chappell left a comment

Choose a reason for hiding this comment

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

🥇

@paperboyo
Copy link
Contributor

@katebee wins Jimmy’s Award for Best Described and Most Informative PRs 2018 hands down!

Congratulations on behalf of The Committee 🥇

@@ -32,6 +32,8 @@
}

@include mq(desktop) {
// IE 10 and 11 do not recognize 'unset' so will fallback to 'auto' which luckily gives us the desired layout
left: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does auto work across the board? If so, maybe we can lose the unset rule altogether?

@katebee katebee merged commit 74498ba into master Jan 4, 2019
@katebee katebee deleted the kw-190103-IE-fixes branch January 4, 2019 10:35
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @katebee 16 minutes and 21 seconds ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants