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

Updated slider #1307

Merged
merged 5 commits into from
Sep 22, 2016
Merged

Updated slider #1307

merged 5 commits into from
Sep 22, 2016

Conversation

aschweigert
Copy link

opening a PR for this to review for 0.5.5

Copy link

@rclations rclations left a comment

Choose a reason for hiding this comment

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

I'm not sure that all of this is related to this PR, but this is the first time I'm reviewing this functionality so I'll drop it all here and we can break out if necessary.

I've highlighted the main content div in gray and the slideshow navigation in blue to make it easier to see.

  1. <div class="hero is-gallery span12"> has a left margin from the span12 class that needs to be overridden. This will center the slideshow & navigation around the post content div
  2. <ul class="slick-dots" style="display: flex;"> has padding: 0 17.0213%; applied to it, which is pushing it to the right.

Aside from those, this looks 1,000x better than what was there before, and especially the navigation on the top

screen shot 2016-09-21 at 11 31 03 am

@aschweigert
Copy link
Author

Also, if this hasn't been tested with the two column story layout, we should double check that as well.

Copy link

@rclations rclations left a comment

Choose a reason for hiding this comment

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

2-column layout has the same problem with the slideshow top grip/navigation, but not the issue with the slideshow itself being bumped over (because there's no 12-col span).

One interesting note here is that the right navigation overflows into the widget area, so I'm thinking we should remove the 40px padding here. It'll mean that the navigation covers the edge of the photo, but I think that's preferable to covering the sidebar.

@Julia67 what's your opinion on this? We could also make the background transparent instead of the current white (or blue in the screenshot), and also move these in, so they're both in the right spot but not cutting off any of the image?

screen shot 2016-09-21 at 5 13 51 pm

@rclations rclations merged commit 24146c4 into develop Sep 22, 2016
@rclations rclations deleted the updated-slider branch September 22, 2016 17:31
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.

3 participants