pattern-lab / patternlab-php-core

This repository provides the core functionality for Pattern Lab. It is meant to be used from within an Edition with a PatternEngine and StarterKit.
http://patternlab.io/
MIT License
43 stars 62 forks source link

Adding support for ishViewportRange #131

Closed EvanLovely closed 7 years ago

EvanLovely commented 7 years ago

This adds support for ishViewportRange that was added to the StyleguideKit v3.5.0 by @bmuenzenmeyer

bmuenzenmeyer commented 7 years ago

@EvanLovely 👍 💯

sghoweri commented 7 years ago

Hey @EvanLovely - can you check to see if this PL ish config works on your end (if not using the new ishViewportRang config option)?

ishMinimum: '240'
ishMaximum: '2600'

I've had that in my base config.yml for a long, long time and just noticed that this seems to have stopped working (max width of 1200px) if I'm using this new update. For what it's worth though, the new config works great -- I just wanted to see if this was a breaking change or not and if we needed to open a ticket to look into this.

bmuenzenmeyer commented 7 years ago

I replaced these keys when I implemented this on Node, FWIW

On Mon, Oct 16, 2017, 4:55 PM Salem notifications@github.com wrote:

Hey @EvanLovely https://github.com/evanlovely - can you check to see if this PL ish config works on your end?

ishMinimum: '240' ishMaximum: '2600'

I've had that in my base config.yml for a long, long time and just noticed that this seems to have stopped working (max width of 1200px) if I'm using this new update. For what it's worth though, the new config works great -- I just wanted to see if this was a breaking change or not and if we needed to open a ticket to look into this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pattern-lab/patternlab-php-core/pull/131#issuecomment-337055305, or mute the thread https://github.com/notifications/unsubscribe-auth/AASNw1tFMyQNNWEA2Mxb7WSp9A4ro1FWks5ss9DEgaJpZM4PzFNu .

coreylafferty commented 7 years ago

@EvanLovely @sghoweri Not sure if this will be helpful, but I was playing around with this in our setup, and this is what I'm seeing:

1) Fresh install adds these to config.yml: `ishViewportRange: s:

With that in place, Pattern Lab is locked to a maximum of 1200px. The 'Full' button will not extend it beyond 1200. The width drag bars will not go beyond 1200.

2) With ishViewportRange values removed (leaving only ishMinimum and ishMaximum), the behavior does not change. Width is still locked to 1200px.

3) Changing the final isViewportRange 'large' value from 1200 to 2600 allows the width to go beyond 1200 and 'Full' works again, setting the iframe width to your viewport width.

sghoweri commented 7 years ago

@coreylafferty I need to look into this further but this is helpful - thank you!

@bmuenzenmeyer which is totally fine if we want to phase out the old config option in PL -- my concern is actually more about us accidentally introducing a major breaking change w/o a deprecation notice or a major version bump

EvanLovely commented 7 years ago

Yeah, looking at your release notes Brian, I see this and it also make me thinks that breaks backwards compatibility and should of necessitated a major version bump.

The first entry in ishViewportRange.s is the ishViewportMinimum, which can now be deleted from your config The second entry in ishViewportRange.l is the ishViewportMaximum, which can now be deleted from your config

~ StyleguideKit v3.5.0 by @bmuenzenmeyer

What I suggest happening is not a major version bump but instead some logic that could work either config setup.

bmuenzenmeyer commented 7 years ago

Correct me if I am wrong but this was introduced as a progressive enhancement and therefore not a breaking change: https://github.com/pattern-lab/styleguidekit-assets-default/blob/v3.5.0/src/js/styleguide.js#L12-L27

    var minViewportWidth = 240;
    var maxViewportWidth = 2600;

    //set minimum and maximum viewport based on confg
    if (config.ishMinimum !== undefined) {
      minViewportWidth = parseInt(config.ishMinimum); //Minimum Size for Viewport
    }
    if (config.ishMaximum !== undefined) {
      maxViewportWidth = parseInt(config.ishMaximum); //Maxiumum Size for Viewport
    }

    //alternatively, use the ishViewportRange object
    if (config.ishViewportRange !== undefined) {
      minViewportWidth = config.ishViewportRange.s[0];
      maxViewportWidth = config.ishViewportRange.l[1];
    }
EvanLovely commented 7 years ago

Fixed in v2.8.10

EvanLovely commented 7 years ago

This is what should get in for the best fix IMO: https://github.com/pattern-lab/styleguidekit-assets-default/pull/99