-
Notifications
You must be signed in to change notification settings - Fork 62
Adding support for ishViewportRange #131
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
Conversation
@EvanLovely 👍 💯 |
Hey @EvanLovely - can you check to see if this PL ish config works on your end (if not using the new ishViewportRang config option)?
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. |
I replaced these keys when I implemented this on Node, FWIW
…On Mon, Oct 16, 2017, 4:55 PM Salem ***@***.***> 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
<#131 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNw1tFMyQNNWEA2Mxb7WSp9A4ro1FWks5ss9DEgaJpZM4PzFNu>
.
|
@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:
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.
|
@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 |
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.
~ 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. |
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];
}
|
Fixed in v2.8.10 |
This is what should get in for the best fix IMO: pattern-lab/styleguidekit-assets-default#99 |
This adds support for
ishViewportRange
that was added to the StyleguideKit v3.5.0 by @bmuenzenmeyer