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

Install screen version update #448

Merged
merged 6 commits into from
Jun 1, 2024

Conversation

Jag-Marcel
Copy link
Member


Description

Changes the installation screen's description to use the new one from iOS 15.0 onwards, instead of from 17.2. Also changed the deployment target to 15.0 to reflect changes in this PR and in #415.

The new install screen will now show up for iOS versions from 15.0 onwards
Changed deployment target to reflect recent changes to the app
Copy link

github-actions bot commented May 29, 2024

Thank you for the pull request!

The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and iOS rooms once you're in. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you!

Maintainer checklist

  • The commit messages for the remote branch should be checked to make sure the contributor's email is set up correctly so that they receive credit for their contribution

    • The contributor's name and icon in remote commits should be the same as what appears in the PR
    • If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for git config user.email in their local Scribe-iOS repo
  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@andrewtavis andrewtavis self-requested a review May 30, 2024 08:02
@andrewtavis
Copy link
Member

Quick check on this, @Jag-Marcel :) I remember we discussed this, but are we sure we want to increase the minimum version here? We could leave it at 13 and just use the old one from 15 on. The way that it's set up here, we have a minimum of 15 and check if 15 is available. I think keeping it at 13 might be best so that it can still be available on as many levels of devices as possible, but maybe I'm also forgetting another reason why we said 15.

Thanks so much for looking into this!

@Jag-Marcel
Copy link
Member Author

Jag-Marcel commented May 30, 2024

@andrewtavis Ah, I wasn't sure how it worked, I didn't know it would lock out older versions from downloading it. I don't think there was anything that necessarily required iOS 15 to function, but there were things like the buttons with the info icon that look much better on 15.
But a thing I thought of that I just checked is that there are actually no devices that support iOS 13 that don't also support iOS 15, so it wouldn't change much for compatibility. Do we want to change the parts with availability checks then?

@andrewtavis
Copy link
Member

This is some great investigating, @Jag-Marcel! Yes, let's remove the availability checks. Also feel free to remove the longer install directions, which is great as the new ones will be much easier to localize 😊

@andrewtavis
Copy link
Member

Hmmmm, but let me merge in the other PR and then you can bring in the changes on main into here? All kinds of conflicts if not. Will do that tonight!

@andrewtavis
Copy link
Member

Other PR and the changelog update for it are done, @Jag-Marcel :) Let me know if you have any questions here!

Without ScribeColors before it, just the key would cause an "Ambiguous use of '.key'" error in the compiler. I assume this was something changed for iOS 14 or 15, the code won't compile unless you specify the used colour library.
- iOS deployment target in project.pbxproj is now correctly set for all keyboards.
- Simplified code that doesn't require the #available tag for the new target
titleEdgeInsets seems to have been deprecated since 15.0, Xcode said it would be ignored in it so it seemed like a good idea to remove it.
@Jag-Marcel
Copy link
Member Author

Changes are done, it's a lot simpler now in places!
As an aside, should we think about using UIButton.Configuration in other places in the future? It's the implementation I used for #415's buttons with the info icon. It has a few benefits like resizing the text in buttons automatically and makes stuff like padding easier, but they wouldn't change much for the buttons we currently have and I don't know how much work converting it over would be

var keyCharColor = UIColor(.keyChar)
var specialKeyColor = UIColor(.keySpecial)
var keyPressedColor = UIColor(.keyPressed)
var keyColor = UIColor(ScribeColor.key)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for also going through and changing color assignments! I agree that it makes sense for us to be explicit with the source :)

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

Thanks for handling all of this, @Jag-Marcel :) Really great to have the care put into this and allow us to move on from so many of the old version checks 😊

@andrewtavis
Copy link
Member

Changes are done, it's a lot simpler now in places! As an aside, should we think about using UIButton.Configuration in other places in the future? It's the implementation I used for #415's buttons with the info icon. It has a few benefits like resizing the text in buttons automatically and makes stuff like padding easier, but they wouldn't change much for the buttons we currently have and I don't know how much work converting it over would be

And re the above: let's not focus on this in the short term, but you'd be welcome to make an issue for this :) Sounds like a medium good first issue :)

@andrewtavis andrewtavis merged commit 375cc00 into scribe-org:main Jun 1, 2024
1 check passed
@Jag-Marcel
Copy link
Member Author

Will do 👍

@Jag-Marcel Jag-Marcel deleted the version-update branch July 11, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants