[frameit] add iPhone 14 device lineup and new device colors #21648

mrvincenzo · 2023-11-18T13:15:58Z

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.
I've added or updated relevant unit tests.

Motivation and Context

Add new frames of iPhone 14 devices

Description

Add new iPhone 14 device frames
Add new colors supported by iOS devices

Testing Steps

Framed iPhone 14 screenshots with the new frames and colors

sathoeni · 2024-01-17T09:34:38Z

IMHO the current device size for an iPhone 14 Pro is not correct in the current master of https://github.com/fastlane/frameit-frames/

Therefore I've created a pull-request with the correct frame size
I've also adapted the logic of framing, to apply rounded corners when framing an iPhone 14.

Maybe you can try follow fastlane-branch that links to a corresponding frame-it/frames repo that matches the correct iPhone 14 Pro size
https://github.com/sathoeni/fastlane/tree/customFrames

If you are fine with the implementation we can close all other pull requests and hopefully it's getting merged

rogerluan

Thanks for this PR @mrvincenzo! 🙏
Just left some minor comments, could you take a look? 🙇

fastlane/swift/Fastlane.swift

frameit/lib/frameit/options.rb

frameit/lib/frameit/device_types.rb

frameit/lib/frameit/commands_generator.rb

This reverts commit 9b6a825.

# Conflicts: # frameit/lib/frameit/device_types.rb

mrvincenzo · 2024-01-23T07:14:53Z

frameit/lib/frameit/device_types.rb

-    IPHONE_14_PRO ||= Frameit::Device.new("iphone-14-pro", "Apple iPhone 14 Pro", 12, [[1178, 2556], [2556, 1178]], 460, Color::PURPLE, Platform::IOS)
-    IPHONE_14_PRO_MAX ||= Frameit::Device.new("iphone14-pro-max", "Apple iPhone 14 Pro Max", 12, [[1290, 2796], [2796, 1290]], 458, Color::PURPLE, Platform::IOS)
+    IPHONE_14_PLUS ||= Frameit::Device.new("iphone14-plus", "Apple iPhone 14 Plus", 12, [[1284, 2778], [2778, 1284]], 458, Color::MIDNIGHT, Platform::IOS)
+    IPHONE_14_PRO ||= Frameit::Device.new("iphone-14-pro", "Apple iPhone 14 Pro", 12, [[1179, 2556], [2556, 1179]], 460, Color::PURPLE, Platform::IOS)


The resolution in master is incorrect. This is the correct resolution [1179, 2556].

@mrvincenzo the resolution on master is correct! I‘ve tested it and generated all production screens with it.
Generate a screenshot wirh snapshot and check yourseelf

@sathoeni what your are saying is inaccurate - iPhone 14 Pro and iPhone 14 Pro Max resolutions in master are incorrect. The fact that it works, doesn't make it correct.

iPhone 14 Pro and Pro Max spec

iPhone 14 Pro

The resolution should be 2556x1179 and not 2556x1178 like in master.
You can also take a screenshot of the iPhone 14 Simulator and check its size. You will discover it is 2556x1179.

iPhone 14 Pro Max

The DPI should be 460 and not 458 like in master.

Thanks @mrvincenzo for the screenshots. Looks like our CI and my local machine produces somehow screenshots with another resolution.
Can you also adapt the size of the screen in the following repo?
https://github.com/fastlane/frameit-frames

PR to frameit-frames: fastlane/frameit-frames#34

@mrvincenzo
I found the issue regarding the screensize
When you take a manual screenshot from the simulator the width of the iPhone 14 Pro is 1179. When you take a screenshot with an UITest (XCUIScreen.main.screenshot()) then it's width is 1178.
I assume it's an XCode bug, I will open a ticket for apple.

We should reopen your issue, I'm sure @getaaron can merge it later, after the issue is fixed in xcode. In the meantime we should keep 1178, so it's works when using fastlane snapshot.

I've opened FB13569779.

with following text:
When using XCUIScreen.main.screenshot() or XCUIApplication().screenshot for generating screenshots for the iPhone 14 Pro the screensize is different (and according to documentation) not correct, compared the taking screenshots manually from the simulator CTRL+S When I take a screenshot manually from the simulator the screensize is 1179 × 2556 pixels When I take a screenshot with XCUIScreen.main.screenshot() the screensize is 1178 × 2556 pixels According to the documentation the screenshot size should be 1179 × 2556! https://developer.apple.com/help/app-store-connect/reference/screenshot-specifications

and following evidence

I believe we should separate these 2 issues:

Adding iPhone 14 device lineup + new device colors, which is mostly what this PR is doing

Fixing the size of the image of the iPhone 14 Pro, which apparently requires other changes (e.g. sathoeni@c5b57b2) plus perhaps changes in the frames in https://github.com/fastlane/frameit-frames (uncertain, to me)

If we proceed with this PR as is, it looks like it will cause regressions if we don't address the other co-related changes simultaneously? Looks like we'd be falling into scope creep, no?

What do you guys think? @getaaron @mrvincenzo @sathoeni

When we merge only this PR we will cause a regression, because the frame dimensions in the file
devices_types.rb correlate to the dimensions in the offset.json in the frameit-frames repo (https://github.com/fastlane/frameit-frames).
If these don't match it's not possible to add a frame to the corresponding screenshot.

The screen dimensions on master of fastlane and `fastlane/frameit-frames' are currently matching and already support the iPhone14 (Pro) series.

Unfortunately it only works when creating screenshots using XCUITest and the fastlane SnapshotHelper.swift. It does not work when taking screenshots manually and running frameit-frames. The reason is a Bug in XCode 15.x that creates wrong screenshots dimensions for the iPhone 14 series.
Currently the master branch uses also the "wrong" dimensions that's why everything works fine with XCode 15.x but not with previous XCode version.
So we already have a regressions issue here.

The long and the short of it:
If we want to fix the current regressions we need to take all of the following changes:

This PR [frameit] add iPhone 14 device lineup and new device colors #21648

sathoeni@c5b57b2

iPhone 14 Pro Frame Width frameit-frames#35

Sorry it's kind of complicated.. I hope you can follow my explanation.
What do you think @rogerluan?

@rogerluan What do you think? Can we proceed on this one?

frameit/lib/frameit/device_types.rb

mrvincenzo · 2024-01-23T12:17:57Z

Thanks for this PR @mrvincenzo! 🙏 Just left some minor comments, could you take a look? 🙇

@rogerluan, I addressed all comments.

rogerluan

👍 Thanks for this, @mrvincenzo !

mrvincenzo · 2024-01-27T16:32:00Z

Can someone with write access permission merge this pull request?

mrvincenzo · 2024-01-28T17:34:44Z

The PR was opened on November 18th and has remained inactive for two months, regardless of the reasons. Time is a valuable resource, including mine. Since I don't see any particular interest in merging this PR, I am closing it. That would be my first and last PR to this repo.

sathoeni · 2024-01-31T11:00:47Z

@mrvincenzo Can you please reopen this Pull-Request
I'm sure we are getting this merged.

I've implemented a workaround for the weird screenshot behaviour in XCode 15.x
sathoeni@c5b57b2

Can you confirm that only the iPhone 14 Pro Size was incorrect?

getaaron · 2024-01-31T12:10:17Z

@mrvincenzo we'd still like to get this merged. Unfortunately we are generally very slow to merge things as most maintainers also only have minimal time to work on fastlane. Please do not see it as a reflection on you or your contribution, we appreciate both very much even if we are slow to react

getaaron · 2024-01-31T12:11:31Z

@sathoeni i think this looks good to merge as-is, do you have any concerns with it before I merge it?

sathoeni · 2024-01-31T12:51:34Z

@sathoeni i think this looks good to merge as-is, do you have any concerns with it before I merge it?

It also looks good to me.
The only concern I have is the issue I've mentioned above.
XCode 15.x (maybe also previous ones) have a Bug that the screenshots taken with XCUIScreen have the wrong size for the iPhone 14 Pro (the only one I'm aware at the moment).
I've implemented a fix in the SnapshotHelper.swift in my fork, because I can't contribute to this one here (no permissions).

If you can cherry-pick my commit and apply it, it would be awesome.
sathoeni@c5b57b2

rogerluan · 2024-02-10T13:41:53Z

Left a comment here: #21648 (comment)

alex-lechner · 2024-04-03T11:50:41Z

What is currently getting in the way of this being merged?
It would be great to have official support instead of modifying the devices_types.rb file whenever I'm updating Fastlane or installing it in a CI.

sathoeni · 2024-04-10T09:55:00Z

@rogerluan Can we move on here? Any feedback on my suggesstion?
#21648 (comment)

mrvincenzo added 3 commits November 18, 2023 15:10

Add new iPhone 14 devices

35d24ed

Add new iPhone frame colors

e1cb93b

Update Fastlane.swift with new color args

9b6a825

rogerluan added topic: swift tool: frameit labels Dec 27, 2023

rogerluan requested changes Jan 23, 2024

View reviewed changes

rogerluan requested a review from lacostej January 23, 2024 01:28

rogerluan removed the topic: swift label Jan 23, 2024

mrvincenzo added 3 commits January 23, 2024 07:49

Revert "Update Fastlane.swift with new color args"

3ce1392

This reverts commit 9b6a825.

Minor updates following PR comments

3aaf66a

Merge branch 'master' into frameit-add-iphone14

080ad40

# Conflicts: # frameit/lib/frameit/device_types.rb

mrvincenzo commented Jan 23, 2024

View reviewed changes

frameit/lib/frameit/device_types.rb Show resolved Hide resolved

Minor updates following PR comments. Continued.

5ba0640

mrvincenzo requested a review from rogerluan January 23, 2024 12:20

rogerluan approved these changes Jan 23, 2024

View reviewed changes

rogerluan changed the title ~~Add iPhone 14 device to frameit~~ [frameit] add iPhone 14 device lineup Jan 23, 2024

rogerluan changed the title ~~[frameit] add iPhone 14 device lineup~~ [frameit] add iPhone 14 device lineup and new device colors Jan 23, 2024

mrvincenzo closed this Jan 28, 2024

getaaron reopened this Jan 31, 2024

mrvincenzo closed this by deleting the head repository May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[frameit] add iPhone 14 device lineup and new device colors #21648

[frameit] add iPhone 14 device lineup and new device colors #21648

mrvincenzo commented Nov 18, 2023 •

edited

sathoeni commented Jan 17, 2024 •

edited

rogerluan left a comment

mrvincenzo Jan 23, 2024 •

edited

sathoeni Jan 23, 2024 •

edited

mrvincenzo Jan 23, 2024 •

edited

sathoeni Jan 23, 2024

mrvincenzo Jan 24, 2024

sathoeni Jan 31, 2024 •

edited

sathoeni Jan 31, 2024 •

edited

rogerluan Feb 10, 2024 •

edited

sathoeni Feb 26, 2024 •

edited

sathoeni Mar 20, 2024

mrvincenzo commented Jan 23, 2024

rogerluan left a comment

mrvincenzo commented Jan 27, 2024

mrvincenzo commented Jan 28, 2024

sathoeni commented Jan 31, 2024

getaaron commented Jan 31, 2024

getaaron commented Jan 31, 2024

sathoeni commented Jan 31, 2024

rogerluan commented Feb 10, 2024 •

edited

alex-lechner commented Apr 3, 2024

sathoeni commented Apr 10, 2024

[frameit] add iPhone 14 device lineup and new device colors #21648

[frameit] add iPhone 14 device lineup and new device colors #21648

Conversation

mrvincenzo commented Nov 18, 2023 • edited

Checklist

Motivation and Context

Description

Testing Steps

sathoeni commented Jan 17, 2024 • edited

rogerluan left a comment

Choose a reason for hiding this comment

mrvincenzo Jan 23, 2024 • edited

Choose a reason for hiding this comment

sathoeni Jan 23, 2024 • edited

Choose a reason for hiding this comment

mrvincenzo Jan 23, 2024 • edited

Choose a reason for hiding this comment

iPhone 14 Pro

iPhone 14 Pro Max

sathoeni Jan 23, 2024

Choose a reason for hiding this comment

mrvincenzo Jan 24, 2024

Choose a reason for hiding this comment

sathoeni Jan 31, 2024 • edited

Choose a reason for hiding this comment

sathoeni Jan 31, 2024 • edited

Choose a reason for hiding this comment

rogerluan Feb 10, 2024 • edited

Choose a reason for hiding this comment

sathoeni Feb 26, 2024 • edited

Choose a reason for hiding this comment

sathoeni Mar 20, 2024

Choose a reason for hiding this comment

mrvincenzo commented Jan 23, 2024

rogerluan left a comment

Choose a reason for hiding this comment

mrvincenzo commented Jan 27, 2024

mrvincenzo commented Jan 28, 2024

sathoeni commented Jan 31, 2024

getaaron commented Jan 31, 2024

getaaron commented Jan 31, 2024

sathoeni commented Jan 31, 2024

rogerluan commented Feb 10, 2024 • edited

alex-lechner commented Apr 3, 2024

sathoeni commented Apr 10, 2024

mrvincenzo commented Nov 18, 2023 •

edited

sathoeni commented Jan 17, 2024 •

edited

mrvincenzo Jan 23, 2024 •

edited

sathoeni Jan 23, 2024 •

edited

mrvincenzo Jan 23, 2024 •

edited

sathoeni Jan 31, 2024 •

edited

sathoeni Jan 31, 2024 •

edited

rogerluan Feb 10, 2024 •

edited

sathoeni Feb 26, 2024 •

edited

rogerluan commented Feb 10, 2024 •

edited