-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add angled carousel component #146
Conversation
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.
Looks pretty good, a few questions inline. Did you work with Julian on the implementation? I had previously handled the wide screen issues this way: https://codepen.io/stevepole/pen/NvGzwM but either can work.
@@ -0,0 +1,183 @@ | |||
@media (min-width: 992px) { |
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.
Why do we need all of these styles for the docs?
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.
Removed. Not needed since the custom styles are added already.
h3, | ||
h4, | ||
h5, | ||
h6 { |
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.
If we need these we should add the utility classes as well (.h1, .h2, .h3 etc). Although I wonder why these are necessary?
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.
Default carousel.less have this styles:
h1, h2, h3, h4, h5, h6 {
color: @carousel-caption-color;
}
These ones in the angled version change the default white colour. They're not necessary but they're useful since the background of the caption is also white.
less/carousel-angled.less
Outdated
|
||
.carousel-caption-item { | ||
width: 100%; | ||
padding: 0 (@spacer * 4); |
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.
Perhaps this padding should be responsive? 32px quite large on mobile.
less/carousel-angled.less
Outdated
font-size: 0.88em; | ||
} | ||
|
||
.circle { |
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.
Not sure this should be part of the styles as it's quite specific to your use case, perhaps better to use hidden-xs, hidden-sm, hidden-md
to hide it for your implementation?
} | ||
|
||
li { | ||
width: @spacer; |
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.
Slightly surprised these styles aren't on something clickable (e.g. an anchor or button), often styling the li container can make a mismatch in clickable area.
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.
Agree. This is the original bootstrap carousel indicator markup and tried to maintain it.
Yes, I catched up with Julian about the animation and the behaviour of the caption. |
d3aa5af
to
86c05c1
Compare
86c05c1
to
4e70fed
Compare
This is a custom angled design for the carousel. The design offers a pleasant experience to slide large images and a caption or call to action related to them.
This design shows a different version on tablets/mobiles, more appropriate for small devices.
The styles are added to the basic carousel ones.
It is working on: https://transferwise.com/gb/compare/