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

STENCIL-2455: Prevents carousel images from being cut off on large sc… #909

Merged
merged 4 commits into from
Feb 8, 2017
Merged

STENCIL-2455: Prevents carousel images from being cut off on large sc… #909

merged 4 commits into from
Feb 8, 2017

Conversation

cristycarpenter
Copy link
Contributor

…reens.

JIRA Ticket
https://jira.bigcommerce.com/browse/STENCIL-2455

What
Switches carousel background-size from "cover" to "100% 100%" which fits the entire image into the slide's viewing area.

Why
Carousel images are currently becoming cut off on the top and bottom at browser widths greater than 1261px.

Caveats
Images may look a bit stretched.

  • We could increase the height set by the hero carousel?
  • We could remove "visibility: hidden" on .heroCarousel-image (although, the image wouldn't scale :/)

Screenshots
Before Change:
stencil-2455-broken

After Change:
stencil-2455-fix

@bigbot
Copy link

bigbot commented Jan 24, 2017

Autotagging @mcampa @bc-miko-ademagic @davidchin

@mcampa
Copy link
Contributor

mcampa commented Jan 24, 2017

We could add a toggle in the Theme Editor to enable this feature 🍹. Something like:

  • Allows image to scale on large screens

This way people won't complain about their images being too stretched. To add the checkbox add a new setting to the schema.json file under Carousel section

@cristycarpenter
Copy link
Contributor Author

I actually had that same thought earlier @mcampa! I forgot to post it as a possible solution. I am comfortable making those code changes if that's what we think is best :)

@mcampa
Copy link
Contributor

mcampa commented Feb 6, 2017

Added two checkboxes:

image

image

@mjschock @junedkazi @mattolson

@@ -745,6 +745,18 @@
"name": "Carousel",
"settings": [
{
"type": "checkbox",
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this being used ? I do not see any corresponding template change to support it.

Copy link
Contributor

@mcampa mcampa Feb 6, 2017

Choose a reason for hiding this comment

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

@@ -9,7 +9,8 @@
}'
>
{{#each carousel.slides}}
<div class="heroCarousel-slide" style="background-image: url({{image}})">
<div
class="heroCarousel-slide" style="background-image: url({{image}})">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to move this to the next line ?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, that was a mistake. Fixing

@@ -75,8 +78,14 @@
}
}

&.heroCarousel-slide-stretch .heroCarousel-slide {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to modify the appearance of a CSS selector, you should use -- instead of -. Also I think this is unneccessarily nested. You can lower its specificity by moving it out. i.e.:

.heroCarousel-slide--stretched {
    @include breakpoint("large") {
        background-size: 100%;
    }
}

And apply the modifier class directly to .heroCarousel-slide :)

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@@ -83,6 +85,12 @@
background-size: cover;
position: relative;

&.heroCarousel-slide--stretch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing the change! heroCarousel-slide--stretch doesn't need to be nested in heroCarousel-slide. Just move the former below the latter.

@mcampa
Copy link
Contributor

mcampa commented Feb 7, 2017

We are now using CHANGELOG.md. Please add an entry with your change under draft before merging

## Draft
- <here>

## 1.5.1 (2017-02-07)
- ...

@nickdengler
Copy link
Contributor

Tested on an integration store 💚

@mcampa mcampa merged commit 8a83d60 into bigcommerce:master Feb 8, 2017
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.

6 participants