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

[frameit] add font_size parameter #16870

Merged
merged 1 commit into from
Jul 21, 2020
Merged

Conversation

ChaosCoder
Copy link
Contributor

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

frameit scales the keyword and title automatically per screenshot to fit the available size above or below the device. This is a good approach for simple screenshots, but lacks a consistent and unified appearance across the screenshots.

Specifying a fixed font_size for keyword and title (separately) gives users of frameit the ability to fix the font size and keep it consistent across screenshots.

Description

This PR introdocudes a new parameter font_size for the keyword and title parameters. Instead of calculating the actual_font_size from the available space everywhere, it now checks for the font_size and returns it if specified and font_size > 0. This way, we keep the old behavior intact.

Testing Steps

Specifiy a font_size in either the default config in your Framefile.json or for a single screenshot config for the title or keyword. Frameit should use this font size when generating the framed screenshots.

Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition, although a bit scary. Sure this will not change existing behavior?

@ChaosCoder
Copy link
Contributor Author

Great addition, although a bit scary. Sure this will not change existing behavior?

The code now checks (see below), if a font_size is configured for either keyword or title and only if it does, it returns that. If not, the old implementation continues. So yes, I'm sure this does not change existing behavior.

       font_size = @config[key.to_s]['font_size']
       return font_size if !font_size.nil? && font_size > 0

@joshdholtz joshdholtz changed the title [frameit] Add font_size parameter [frameit] add font_size parameter Jul 20, 2020
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small safety check!

frameit/lib/frameit/editor.rb Show resolved Hide resolved
Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thank you so much for adding this and responding to my review! Really appreciate the contribute ❤️

@joshdholtz joshdholtz merged commit e53bd3a into fastlane:master Jul 21, 2020
@fastlane-bot
Copy link

Hey @ChaosCoder 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congratulations! 🎉 This was released as part of fastlane 2.153.0 🚀

@fastlane fastlane locked and limited conversation to collaborators Sep 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants