-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(demo): direct link to specific carousel sample not working #3272
fix(demo): direct link to specific carousel sample not working #3272
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3272 +/- ##
=======================================
Coverage 90.77% 90.77%
=======================================
Files 95 95
Lines 2711 2711
Branches 507 507
=======================================
Hits 2461 2461
Misses 190 190
Partials 60 60
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @divdavem. LGTM !
@fbasso Thank you for your review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
Nonetheless, I left a little remark that we should address I think.
demo/src/style/demos.css
Outdated
@@ -41,3 +41,17 @@ ngbd-table-filtering span.ngb-highlight { | |||
ngbd-table-complete span.ngb-highlight { | |||
background-color: yellow; | |||
} | |||
|
|||
.picsum-img-wrapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicking here, but I would actually have prefixed that selector with ngb-caroussel
so that we know it's used inside caroussel demos just by reading here.
ngb-caroussel .picsum-img-wrapper {
}
demo/src/style/demos.css
Outdated
padding-top: 55%; /* Keep ratio for 900x500 images */ | ||
} | ||
|
||
.picsum-img-wrapper>img { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some remark as above.
The carousel samples in the demo site contain images that are loaded asynchronously. But as their size is not reserved beforehand, they shift the remaining content when loaded. As a consequence, the scrolling position that was set correctly to target a specific sample before the images are loaded is no longer correct after the images are loaded. This commit reserves the space for the images by using the trick documented in the following link to also keep the responsive behavior: https://itnext.io/how-to-stop-content-jumping-when-images-load-7c915e47f576
74c9c69
to
0b69f1c
Compare
@benouat Thank you for your review! I have prefixed the two css rules with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@benouat Thank you for merging my PR! |
The carousel samples in the demo site contain images that are loaded asynchronously. But as their size is not reserved beforehand, they shift the remaining content when loaded. As a consequence, the scrolling position that was set correctly to target a specific sample before the images are loaded is no longer correct after the images are loaded.
This PR reserves the space for the images by using the trick documented in the following link to also keep the responsive behavior:
https://itnext.io/how-to-stop-content-jumping-when-images-load-7c915e47f576
Before submitting a pull request, please make sure you have at least performed the following: