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

feat(alert): update styles to iOS 17 specs #28726

Merged
merged 15 commits into from
Dec 20, 2023
Merged

feat(alert): update styles to iOS 17 specs #28726

merged 15 commits into from
Dec 20, 2023

Conversation

averyjohnston
Copy link
Contributor

@averyjohnston averyjohnston commented Dec 18, 2023

Issue number: N/A


What is the current behavior?

Alerts don't match the iOS 17 spec for styling.

What is the new behavior?

  • Overlay border radius increased.
  • Input font size increased.
  • Input padding increased.

Does this introduce a breaking change?

  • Yes
  • No

Other information

The difference in the MD screenshots is the box shadow on the header in the background. This was a change made a while back that probably wasn't caught due to the pixel threshold on the screenshot update job. Deleting all screenshots and running again got it updated.

@github-actions github-actions bot added the package: core @ionic/core package label Dec 18, 2023
@averyjohnston averyjohnston marked this pull request as ready for review December 18, 2023 20:26
@averyjohnston averyjohnston requested a review from a team as a code owner December 18, 2023 20:26
@averyjohnston averyjohnston requested review from liamdebeasi and removed request for a team December 18, 2023 20:26
@sean-perkins sean-perkins changed the title fix(alert): update styles to iOS 17 specs feat(alert): update styles to iOS 17 specs Dec 18, 2023
@averyjohnston
Copy link
Contributor Author

One sec, need to add a border radius update that was missed in the design doc 👀

@averyjohnston averyjohnston marked this pull request as draft December 18, 2023 21:02
@averyjohnston averyjohnston removed the request for review from sean-perkins December 18, 2023 21:02
@averyjohnston
Copy link
Contributor Author

Okay, should be good now 👍

@averyjohnston averyjohnston marked this pull request as ready for review December 18, 2023 21:42
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Dec 19, 2023

After discussing with Amanda offline, I made a few adjustments so the alert inputs can be more closely aligned. It's likely that there are other areas where we do not perfectly align with the spec. However, I opted to focus on the inputs and wrapper border radius since that was the agreed upon scope.

I tested this by overlaying an Ionic alert with a native alert. The following image verifies that the wrapper border radius value is aligned:

Screenshot 2023-12-19 at 4 20 53 PM

The following image verifies that the input styles are aligned:

On this following image, the top/bottom padding on native uses sub pixel value, but I opted not to do that here for rendering consistency. As a result, there is a bit of blurriness on the top/bottom edges of the overlaid input.

Screenshot 2023-12-19 at 4 21 03 PM

The native demo was derived using the following Swift UI code and tested against an iPhone 13 running iOS 17:

struct ContentView: View {
    @State private var presentAlert = false
    var body: some View {
        Button("Show Alert") {
            presentAlert = true
        }
        .alert("Title", isPresented: $presentAlert, actions: {
            // Any view other than Button would be ignored
            TextField("TextField", text: .constant("Value"))
        }, message: {
            // Any view other than Text would be ignored
            TextField("TextField", text: .constant("Value"))
        })
    }
}

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I've made my changes. I verified them against a real iOS device, but I'd appreciate it if you could take a quick look too before you merge 😄

@averyjohnston
Copy link
Contributor Author

Looks great to me, thanks again!

@averyjohnston averyjohnston merged commit 979b2f6 into FW-4845 Dec 20, 2023
44 checks passed
@averyjohnston averyjohnston deleted the FW-5732 branch December 20, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants