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

test: make all buttons visible/tappable #2828

Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Mar 23, 2023

We've added enough buttons to the sample app now that on small devices like the iphone 8, they aren't all tappable.

I made a couple stopgap changes to fix this:

  • add a third column of buttons
  • make all button text smaller
  • pin all edges of outermost stack view to safe area layout margins, to keep it from going up underneath the navbar (this was the actual issue, it was obscuring a button a ui test was trying to tap, thereby failing the test)

Long term we need to reorganize this. Either multiple vcs with related options that can be pushed/popped on the nav controller (my preference), or a long table view or embed the stack view in a scroll view.

Before on an iPhone 8, which we use in saucelabs:
image

After:
image

#skip-changelog

@github-actions
Copy link

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1209.10 ms 1221.32 ms 12.22 ms
Size 20.76 KiB 425.80 KiB 405.04 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8f397a7 1196.55 ms 1226.82 ms 30.27 ms
8f397a7 1252.37 ms 1274.80 ms 22.43 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
ce4cfaf 1203.61 ms 1218.86 ms 15.25 ms
06548c0 1226.71 ms 1252.37 ms 25.66 ms
4259afd 1222.12 ms 1249.74 ms 27.62 ms
7fb7afb 1235.00 ms 1256.81 ms 21.81 ms
369222e 1232.14 ms 1258.90 ms 26.76 ms
c9724f9 1199.38 ms 1229.54 ms 30.16 ms
28333b6 1247.29 ms 1262.51 ms 15.22 ms

App size

Revision Plain With Sentry Diff
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
ce4cfaf 20.76 KiB 423.19 KiB 402.43 KiB
06548c0 20.76 KiB 427.36 KiB 406.59 KiB
4259afd 20.76 KiB 419.70 KiB 398.94 KiB
7fb7afb 20.76 KiB 419.69 KiB 398.94 KiB
369222e 20.76 KiB 419.67 KiB 398.91 KiB
c9724f9 20.76 KiB 427.66 KiB 406.90 KiB
28333b6 20.76 KiB 424.69 KiB 403.93 KiB

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #2828 (97dec69) into main (9d56232) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2828      +/-   ##
==========================================
- Coverage   81.30%   81.29%   -0.02%     
==========================================
  Files         258      258              
  Lines       24131    24116      -15     
  Branches    10706    10698       -8     
==========================================
- Hits        19619    19604      -15     
  Misses       4014     4014              
  Partials      498      498              

see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d56232...97dec69. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

I would rather introduce a ScrollView, Is a much simpler and future proof solution.
The tiny font and third column doest not look good to me.

@philipphofmann
Copy link
Member

I agree with @brustolin that the small font size is not the best idea, as I think tapping the buttons can get complicated, but a scroll view might also not be the best for UI tests. We could also split the screen into multiple ones and add some navigation.

@armcknight
Copy link
Member Author

I agree a scroll view will be complicated, that would be my last choice.

Since this problem blocks #2823, would you be ok merging this for now and then circling back within a couple days for a more robust solution? I'm happy to break it up into screens with navigation, I can take that on in a few days. We can have it in by the middle of next week.

It will just take more significant refactoring of logic in ViewController.swift and of UI tests, which is why I hoped to just get by with the smaller buttons for the time being.

@brustolin
Copy link
Contributor

I dont mind. Sample should not be a blocker for the SDK.

Btw, is a good opportunity to introduce UITabViewController in the sample.
Organize the actions by feature.

@armcknight armcknight changed the title test: make all buttons tappable test: make all buttons visible/tappable Mar 24, 2023
@armcknight armcknight merged commit 725565a into main Mar 24, 2023
@armcknight armcknight deleted the armcknight/test/make-sample-app-buttons-all-tappable branch March 24, 2023 18:59
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.

3 participants