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

Adding support for ishViewportRange #131

Merged
merged 2 commits into from
Oct 9, 2017

Conversation

EvanLovely
Copy link
Member

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

@EvanLovely EvanLovely merged commit 3a82565 into master Oct 9, 2017
@bmuenzenmeyer
Copy link
Member

@EvanLovely 👍 💯

@sghoweri
Copy link
Contributor

sghoweri commented Oct 16, 2017

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
Copy link
Member

bmuenzenmeyer commented Oct 16, 2017 via email

@coreylafferty
Copy link

@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: - 320 - 500 m: - 500 - 800 l: - 800 - 1200 ishMinimum: '240' ishMaximum: '2600'

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.

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

  2. 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
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Member

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];
    }
  • preserves defaults as hardcoded fallbacks
  • uses ishMin/Max if present
  • uses ishViewportRange if present

@EvanLovely
Copy link
Member Author

Fixed in v2.8.10

@EvanLovely
Copy link
Member Author

This is what should get in for the best fix IMO: pattern-lab/styleguidekit-assets-default#99

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.

None yet

4 participants