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

Jump to bottom

[deliver] add support to detecting and uploading 6.7" (iPhone 14 Pro Max) screenshots #20694

Merged
merged 12 commits into from Sep 30, 2022
Merged

[deliver] add support to detecting and uploading 6.7" (iPhone 14 Pro Max) screenshots #20694

merged 12 commits into from Sep 30, 2022
+39 −0

Conversation

cherpake
Copy link
Contributor

@cherpake cherpake commented Sep 25, 2022 •
edited by fastlane-bot

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 see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Description

Added new screenshot sizes for 6.7inch iPhone 14 Pro Max

Testing Steps

@google-cla
Copy link

google-cla bot commented Sep 25, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

rogerluan
rogerluan reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Thank you for this, @cherpake ! Have you confirmed it works by uploading 6.7" screenshots to the App Store?

Would you mind signing the CLA with Google? The instructions were posted by the bot, right above :)

Thanks once again!

deliver/spec/app_screenshot_spec.rb Outdated Show resolved Hide resolved
fastlane/lib/fastlane/version.rb Outdated Show resolved Hide resolved
@rogerluan rogerluan changed the title upload 6.7inch screenshots [deliver] add support to detecting and uploading 6.7" (iPhone 14 Pro Max) screenshots Sep 26, 2022
rogerluan
rogerluan requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

Could you also check for linter issues by running bundle exec rubocop -a? 🙏

giginet
giginet reviewed Sep 29, 2022
View reviewed changes
Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

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

I just need this feature. So I'll try this patch. Good job ✨

LGTM except for existing comments.
Could you fix them?

@cherpake
Copy link
Contributor Author

Done, fixed one issue

giginet
giginet requested changes Sep 30, 2022
View reviewed changes
Copy link
Collaborator

@giginet giginet left a comment •
edited

Choose a reason for hiding this comment

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

@cherpake
Could you revert the changes for Gemfile.lock?

And I pointed out minor fixes.

After you fixed, it will be merged soon.

Gemfile.lock Outdated
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
fastlane (2.210.1)
fastlane (2.210.2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you revert Gemfile.lock?
This file should not be changed by this PR.

deliver/lib/deliver/app_screenshot.rb Show resolved Hide resolved
deliver/lib/deliver/app_screenshot.rb Outdated Show resolved Hide resolved
deliver/spec/app_screenshot_spec.rb Outdated Show resolved Hide resolved
cherpake and others added 3 commits September 30, 2022 08:17
@cherpake @giginet
Update deliver/lib/deliver/app_screenshot.rb
a758f8a 
Co-authored-by: Kohki Miki <giginet.net@gmail.com>
@cherpake @giginet
Update deliver/spec/app_screenshot_spec.rb
4ddcc64 
Co-authored-by: Kohki Miki <giginet.net@gmail.com>
@cherpake @giginet
Update deliver/lib/deliver/app_screenshot.rb
673e1b9 
Co-authored-by: Kohki Miki <giginet.net@gmail.com>
@giginet
Copy link
Collaborator

giginet commented Sep 30, 2022 •
edited

@cherpake I'm so sorry for your inconvenience 🙏 our CI avoided placing trailing commas.
So my suggestions were completely wrong 🙇
Could you revert them?

673e1b9
a758f8a

cherpake and others added 2 commits September 30, 2022 08:32
@cherpake @giginet
Update deliver/lib/deliver/app_screenshot.rb
768997e 
Co-authored-by: Kohki Miki <giginet.net@gmail.com>
@cherpake @giginet
Update deliver/lib/deliver/app_screenshot.rb
75d25ab 
Co-authored-by: Kohki Miki <giginet.net@gmail.com>
@cherpake
Copy link
Contributor Author

I want Fastlane t-shirt for this :)

@giginet
Copy link
Collaborator

giginet commented Sep 30, 2022 •
edited

Could you revert Gemfile.lock?
https://github.com/fastlane/fastlane/pull/20694/files#r984199848

You just restore Gemfile.lock.
git restore -s master Gemfile.lock

@cherpake
Copy link
Contributor Author

Done...

giginet
giginet approved these changes Sep 30, 2022
View reviewed changes
Copy link
Collaborator

@giginet giginet left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution 😄
I'll merge this after checking with other members.

rogerluan
rogerluan requested changes Sep 30, 2022
View reviewed changes
deliver/lib/deliver/app_screenshot.rb Outdated Show resolved Hide resolved
rogerluan
rogerluan approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

🚀 LGTM! Thank you again for this PR @cherpake !
(and I wish we had fastlane T-shirts too! ❤️ )

@giginet giginet merged commit 950bb82 into fastlane:master Sep 30, 2022
@fastlane-bot
Copy link

Hey @cherpake 👋

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 🚀

@joshdholtz joshdholtz mentioned this pull request Nov 12, 2022
Merged
@fastlane-bot
Copy link

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

@fastlane-bot fastlane-bot mentioned this pull request Nov 16, 2022
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giginet giginet giginet approved these changes

@rogerluan rogerluan rogerluan approved these changes

Assignees
No one assigned
Labels
status: released tool: deliver
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cherpake @giginet @fastlane-bot @rogerluan