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

Implemented overflow fix for preloaded container #153

Merged
merged 2 commits into from
Jan 12, 2018
Merged

Implemented overflow fix for preloaded container #153

merged 2 commits into from
Jan 12, 2018

Conversation

craigpryde
Copy link
Contributor

What does this PR cover?

Implemented a css update to fix overflow issue detailed in: #152

How can this be tested?

Compile and run

Reviewers

Review 1

  • 👍

By adding a +1 you are confirming you have...

  • Witnessed the work behaving as expected (this could be on the author's machine or screencast).
  • Checked for coding anti-patterns.

@Andy-set-studio
Copy link
Contributor

Hey @craigpryde! Thanks so much for finding this and putting in a PR. We really appreciate it.

It looks like a fair enough fix, but how come you've set .ndpl-preloaded to display: table and not left/set it as display: block?

The overflow rule should work because of the absolute positioning that's been set in my opinion.

There's a good chance that I'm being ignorant, but it would be great to know :)

@craigpryde
Copy link
Contributor Author

Hey @hankchizljaw,

Thanks for the response. I did try display:block; on the preloaded container but this appeared to have no effect on the component and it still rendered in the same way.
image

I suspect this is due to the component not having a display property set which would mean it would be set to display:block; on chrome by default.
image

It was after trying display block that I changed the display to table to force the browser to render the container differently. I'm sure the display block; solution would work under most circumstances and would be my first pick also but from my experience, it can cause rendering issues like the above when used with a lot of flexbox based child elements causing the overflow.

I did, however, dive deeper into the issue and found adding width:0px; onto the element with display block also solved the overlapping issue but this resulted in the components displaying a "This component is hidden at this resolution." message that must have been caused by the JS when the hidden content is preloaded.
image

Because of that, I chose to use the display: table; property as this seemed to rectify the bug and from other projects, I have found this to be the most reliable.

Would be great to know your thoughts :)

@Andy-set-studio
Copy link
Contributor

Hey @craigpryde, so sorry about the delay!

I think that's all fair enough and well-balanced.

Let's merge it in 🚀

@gaetanmarsault
Copy link

Hi everbody, the solution (display: table on .ndpl-preloaded) seems not working in IE 11... have you got any idea ? Thx

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