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

Add option to select color of device frame #15

Closed
wants to merge 6 commits into
base: dev
from

Conversation

Projects
2 participants
@zanuka
Copy link

zanuka commented Mar 6, 2019

  • updated sketch files
  • created new transparent png's with matte / silver chrome

@mmcc007 this PR is just a suggestion to support all the various chromes I saw you had in the sketch template files. could also be cool to add configs in the screens.yaml for all the various options.

happy to revise this or disregard it if you already had this in progress

  • Mike
@mmcc007

This comment has been minimized.

Copy link
Owner

mmcc007 commented Mar 6, 2019

Thanks for the PR!

Currently there is no choice for device frames, it's whatever the default is in screens.yaml.

Providing frame options is a good thing.

This would require adding a feature for over-riding a screen's default frame with provided alternatives. This would probably be configurable from the screenshots.yaml.

Facebook publishes the sketch files and pngs device frame alternatives for all the screen sizes. So for frames, the art is out there.

Let's say device frames are consistently Chrome (for the selected android and iOS devices) across all currently supported screen sizes.

Say we want to add Black as a color option. We would have to provide the black frames for all screen sizes and document how to change frame color in README.

To implement should be a matter of:

  1. For each new frame color:
    1. Add sketch file to assets (just in case)
    2. Add image for new frame option to package resources
  2. In code:
    1. Unpack frame to local disk
    2. Feed option over-ride to ImageMagick

Then to trigger this code add something like the following to screenshots.yaml

ios:
  5.5inch:
    frame: resources/ios/phones/iPhone_7_Plus_Black.png
@zanuka

This comment has been minimized.

Copy link
Author

zanuka commented Mar 6, 2019

No problem, happy to help. I decided to fork the repo since we needed the black frames for our app submission and thought it might be useful to PR back in, get the conversation going...

Your implementation approach sounds good. I can continue on that path and update this PR accordingly. If you have a particular workflow process let me know (e.g. prefer creating feature branches).

Also noticed you released v1.1.5 👍

@mmcc007

This comment has been minimized.

Copy link
Owner

mmcc007 commented Mar 6, 2019

Great!

The default frame color is 'Silver' for iOS and 'Black' for android at the moment.

It might make sense to default to a single default color like Black for both iOS and android (might ultimately be easier for users to follow?). Users could then pick and choose which screens should have which alternative color (eventually can add other color selection methods, like 'Silver for iOS', etc...)... Ultimately, I leave it up to you.. maybe makes more sense to see how color feature works first before tweaking it some more if necessary.

BTW: should probably add checks to the Config validator, to confirm that color options are valid before starting a run.

Merging the PR into dev (as you are doing) seems like a good way to go. I will then later merge it into master.

Thanks for taking a stab at this... should be a cool feature.

@zanuka

This comment has been minimized.

Copy link
Author

zanuka commented Mar 7, 2019

Ok cool, that sounds good to me. I'll keep pushing ahead on it

@mmcc007 mmcc007 added this to In progress in Status Mar 7, 2019

@mmcc007 mmcc007 moved this from In progress to To do in Status Mar 7, 2019

@mmcc007 mmcc007 moved this from To do to In progress in Status Mar 7, 2019

@mmcc007 mmcc007 changed the title adds black device chrome :: iPhone 7 Plus / iPad Pro Add option to select color of device frame Mar 7, 2019

@mmcc007 mmcc007 added the enhancement label Mar 7, 2019

@mmcc007

This comment has been minimized.

Copy link
Owner

mmcc007 commented Mar 7, 2019

BTW: in case you don't already know, the artwork for the frames is here:
https://facebook.design/devices

I've been using the frame pngs (and sketch files) from there. Should be a good starting point for adding frame colors for each device.

@zanuka

This comment has been minimized.

Copy link
Author

zanuka commented Mar 11, 2019

Yeah I thought I had included my modified sketch source files on this PR. Agreed that will be best approach for all the frame artwork. I did notice the top of the frames could use some margin. Our app was recently approved and the screenshots look a bit cut off at the top and bottom so I'm going to make some tweaks and try again.

@mmcc007

This comment has been minimized.

Copy link
Owner

mmcc007 commented Mar 11, 2019

Right... imageMagick will center and scale the frame in the 'canvas' (which is the size of the original screenshot). You can tweak the scaling (down to the pixel level) using the 'resize' parameter in screens.yaml.

@zanuka

This comment has been minimized.

Copy link
Author

zanuka commented Mar 11, 2019

Cool, I figured that's what the percentages were there. How about offset values? I see you have offset values doing something similar, was that just by trial & error or using some recommended settings?

@zanuka

This comment has been minimized.

Copy link
Author

zanuka commented Mar 11, 2019

I'll just close this PR for now and re-open once I feel it's ready to go. Glad to get the discussion going and maybe we could transfer some of this to an issue, to document more precise requirements, which device frames to support (vote?), etc... I don't have much time this week but will see what I can do.

@zanuka zanuka closed this Mar 11, 2019

Status automation moved this from In progress to Done Mar 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.