marcisme / sketch-preview

Sketch plugin to preview mockups in Skala Preview
MIT License
611 stars 37 forks source link

Scaling modes by width only #13

Closed areohbe closed 9 years ago

areohbe commented 9 years ago

It seems like it would make sense to set device scaling to scale the artboard's width to match device size (e.g. 375pt to 1242pt for 6+) and scale the height relative to actual height of the artboard to accommodate artboards that exceed the device screen height.

marcisme commented 9 years ago

I thought about this some while making the most recent changes, but I decided to defer when considering how to best handle landscape, since content could scroll on either axis.

The most reasonable thing to do is probably to look at one dimension being equal to a known screen size, with the other dimension being greater than or equal.

Can you think of any cases where that would be invalid?

areohbe commented 9 years ago

Seems like adding portrait and landscape options for each scaling mode would solve for that?

marcisme commented 9 years ago

Yes, but I'd prefer to spare users from having to change settings if they don't have to. I'll think through the various permutations and try to come up with something this weekend.

Thanks for bringing it up.

marcisme commented 9 years ago

I think I have this working. Could you check out the scrollable branch?

areohbe commented 9 years ago

Hmm... "Preview Setup" isn't working for me via the keyboard shortcut or from the menu bar.

marcisme commented 9 years ago

Double-check that there's only the latest version of the plugin in the plugins directory. I switched from a couple files to a directory with all the stuff in it, so it's possible there's an old version in there that's mixing things up.

areohbe commented 9 years ago

Yup, I made sure to delete any files from the previous version before installing. Here's what I'm seeing after enabling debug.

image

marcisme commented 9 years ago

Weird. It shouldn't be possible to get an out of range value in there, but here we are. I've added some code to enforce default values if the index is out of bounds for whatever reason. Give it a try now.

marcisme commented 9 years ago

Also try changing the options a few times and making sure they're saved correctly. I'm curious if there's some other issue that's causing the invalid value to be saved, but I couldn't find anything suspicious.

areohbe commented 9 years ago

Hmm, setup dialog is working now but it looks like the chosen size is not being honored. Here's what I'm seeing.

image

Looks like the scaling modes may not be getting saved. It's set to explicit 2x every time I open the dialog.

marcisme commented 9 years ago

The config values get written out to a file in Sketch's container, and default values are supposed to be used if nothing's been explicitly set yet. Weirdly the default is explicit 1x, so I'm not sure why 2x would be getting chosen. There must be either a problem reading/writing to that file, or perhaps the file has been corrupted.

Do you have the App Store or direct download version of Sketch? I assume this is with 3.2?

Can you look for a config.plist file at ~/Library/Containers/com.bohemiancoding.sketch3/Data/Library/Caches/com.marcisme.sketch-preview and paste the contents in here?

It should look something like this:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
        <key>previewSizeIndex</key>
        <integer>1</integer>
        <key>scalingStrategyId</key>
        <integer>2</integer>
</dict>
</plist>
areohbe commented 9 years ago

I'm running 3.2 from the App Store. Here's what config.plist looks like:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
    <key>previewSizeIndex</key>
    <integer>3</integer>
    <key>scalingStrategyId</key>
    <integer>4</integer>
</dict>
</plist>

 

marcisme commented 9 years ago

The config file looks fine, and you're using the same version as me. I pushed up some additional debug logging to print out the various file paths and config values. Please try again with DEBUG on and paste the logs here. (I moved the logging stuff to common.js)

areohbe commented 9 years ago

Here's what I'm seeing:

image

marcisme commented 9 years ago

I think I got it. When I fixed the out of bounds error from the earlier post with NSSegmentedCell showing up in the log, I "proactively" applied that bounds checking to another config value. Except that value wasn't actually an index, so when the 6+ mode was selected, it would be out of the array range and be set back to the default value. Still no idea how -2 ended up in the config file, but it should work now.

areohbe commented 9 years ago

Works great! Thanks for all the help, Marc.

marcisme commented 9 years ago

Awesome. Thanks for helping test the changes.