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

Feedback modal UI tweaks #4492

Open
wants to merge 11 commits into
base: feedback-ui
Choose a base branch
from
Open

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Jan 30, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

Based on #4435

📜 Description

  • Disables FeedbackForm bouncing
  • Adds sheet appearance for the modal

💡 Motivation and Context

Part of #4302

💚 How did you test it?

Manual

Screenshots
iPhone 16 Pro with notch iPhone SE without notch
Simulator Screenshot - iPhone 16 Pro - 2025-01-30 at 16 14 30 Simulator Screenshot - iPhone SE (3rd generation) - 2025-01-30 at 16 14 24
Pixel 8 Pro with camera notch Pixel 2 with bottom buttons
Screenshot_20250130_161359 Screenshot_1738246455

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog

Copy link
Contributor

github-actions bot commented Jan 30, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 433.59 ms 508.79 ms 75.20 ms
Size 7.15 MiB 8.39 MiB 1.23 MiB

Baseline results on branch: feedback-ui

Startup times

Revision Plain With Sentry Diff
e5d5735+dirty 377.37 ms 430.04 ms 52.67 ms
8cb898b+dirty 393.33 ms 416.20 ms 22.87 ms
3e4cdf5+dirty 642.13 ms 702.23 ms 60.10 ms
269c976+dirty 395.13 ms 438.37 ms 43.24 ms
0325426+dirty 418.89 ms 485.00 ms 66.11 ms
2646c98+dirty 415.13 ms 438.41 ms 23.28 ms
0459aee+dirty 424.10 ms 466.63 ms 42.53 ms

App size

Revision Plain With Sentry Diff
e5d5735+dirty 7.15 MiB 8.39 MiB 1.23 MiB
8cb898b+dirty 7.15 MiB 8.39 MiB 1.24 MiB
3e4cdf5+dirty 7.15 MiB 8.39 MiB 1.23 MiB
269c976+dirty 7.15 MiB 8.39 MiB 1.23 MiB
0325426+dirty 7.15 MiB 8.38 MiB 1.23 MiB
2646c98+dirty 7.15 MiB 8.38 MiB 1.23 MiB
0459aee+dirty 7.15 MiB 8.38 MiB 1.23 MiB

Previous results on branch: antonis/feedback-modal-ui

Startup times

Revision Plain With Sentry Diff
8216878+dirty 347.88 ms 363.18 ms 15.30 ms
de197fe+dirty 354.64 ms 397.88 ms 43.23 ms
5bd21c5+dirty 355.61 ms 403.40 ms 47.79 ms

App size

Revision Plain With Sentry Diff
8216878+dirty 7.15 MiB 8.39 MiB 1.24 MiB
de197fe+dirty 7.15 MiB 8.39 MiB 1.24 MiB
5bd21c5+dirty 7.15 MiB 8.39 MiB 1.24 MiB

Copy link
Contributor

github-actions bot commented Jan 30, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1218.14 ms 1223.16 ms 5.02 ms
Size 2.63 MiB 3.71 MiB 1.07 MiB

Baseline results on branch: feedback-ui

Startup times

Revision Plain With Sentry Diff
0325426+dirty 1228.88 ms 1229.92 ms 1.04 ms
0459aee+dirty 1232.82 ms 1231.19 ms -1.63 ms
3e4cdf5+dirty 1222.53 ms 1224.42 ms 1.89 ms
2646c98+dirty 1218.51 ms 1218.92 ms 0.41 ms
e5d5735+dirty 1222.02 ms 1222.22 ms 0.20 ms
269c976+dirty 1210.02 ms 1204.46 ms -5.56 ms
8cb898b+dirty 1221.40 ms 1231.78 ms 10.37 ms

App size

Revision Plain With Sentry Diff
0325426+dirty 2.63 MiB 3.69 MiB 1.06 MiB
0459aee+dirty 2.63 MiB 3.69 MiB 1.06 MiB
3e4cdf5+dirty 2.63 MiB 3.69 MiB 1.06 MiB
2646c98+dirty 2.63 MiB 3.69 MiB 1.06 MiB
e5d5735+dirty 2.63 MiB 3.69 MiB 1.06 MiB
269c976+dirty 2.63 MiB 3.69 MiB 1.06 MiB
8cb898b+dirty 2.63 MiB 3.71 MiB 1.08 MiB

Previous results on branch: antonis/feedback-modal-ui

Startup times

Revision Plain With Sentry Diff
5bd21c5+dirty 1232.90 ms 1229.58 ms -3.31 ms
de197fe+dirty 1222.35 ms 1224.57 ms 2.22 ms
8216878+dirty 1220.35 ms 1222.86 ms 2.51 ms

App size

Revision Plain With Sentry Diff
5bd21c5+dirty 2.63 MiB 3.71 MiB 1.08 MiB
de197fe+dirty 2.63 MiB 3.71 MiB 1.08 MiB
8216878+dirty 2.63 MiB 3.71 MiB 1.08 MiB

@antonis antonis marked this pull request as ready for review January 30, 2025 14:32
Copy link
Contributor

github-actions bot commented Jan 30, 2025

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 437.53 ms 465.48 ms 27.95 ms
Size 17.75 MiB 20.12 MiB 2.37 MiB

Baseline results on branch: feedback-ui

Startup times

Revision Plain With Sentry Diff
8cb898b 438.83 ms 420.58 ms -18.25 ms
0459aee 491.48 ms 486.13 ms -5.35 ms
0325426 477.32 ms 457.43 ms -19.89 ms
3e4cdf5 462.35 ms 474.96 ms 12.61 ms
e5d5735 452.70 ms 453.04 ms 0.34 ms
2646c98 429.98 ms 421.63 ms -8.35 ms
269c976 448.08 ms 428.86 ms -19.22 ms

App size

Revision Plain With Sentry Diff
8cb898b 17.75 MiB 20.12 MiB 2.37 MiB
0459aee 17.75 MiB 20.12 MiB 2.37 MiB
0325426 17.75 MiB 20.12 MiB 2.37 MiB
3e4cdf5 17.75 MiB 20.12 MiB 2.37 MiB
e5d5735 17.75 MiB 20.12 MiB 2.37 MiB
2646c98 17.75 MiB 20.12 MiB 2.37 MiB
269c976 17.75 MiB 20.12 MiB 2.37 MiB

Previous results on branch: antonis/feedback-modal-ui

Startup times

Revision Plain With Sentry Diff
de197fe 471.60 ms 457.30 ms -14.30 ms
5bd21c5 422.60 ms 437.37 ms 14.76 ms
8216878 434.78 ms 431.30 ms -3.48 ms

App size

Revision Plain With Sentry Diff
de197fe 17.75 MiB 20.12 MiB 2.37 MiB
5bd21c5 17.75 MiB 20.12 MiB 2.37 MiB
8216878 17.75 MiB 20.12 MiB 2.37 MiB

@krystofwoldrich
Copy link
Member

The auto-injected form has double the border of the manually created one. That seems to me like a bug.

@krystofwoldrich
Copy link
Member

I would like to iterate over the possible implementations of this.

I think the shadow should not slide up with the modal, it doesn't look natural.

Could we make use of the fact we wrap the modal in view and make the wrapper view fade in out the shadow as the modal slides up?

@krystofwoldrich
Copy link
Member

I've tried the code on iOS and the form does not avoid the on-screen keyboard.

Copy link
Contributor

github-actions bot commented Jan 31, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.14 ms 1225.31 ms -9.83 ms
Size 3.19 MiB 4.27 MiB 1.09 MiB

Baseline results on branch: feedback-ui

Startup times

Revision Plain With Sentry Diff
0325426+dirty 1210.17 ms 1216.37 ms 6.20 ms
0459aee+dirty 1233.67 ms 1239.80 ms 6.12 ms
3e4cdf5+dirty 1213.36 ms 1221.31 ms 7.95 ms
2646c98+dirty 1239.94 ms 1246.90 ms 6.96 ms
e5d5735+dirty 1217.78 ms 1221.80 ms 4.02 ms
269c976+dirty 1223.29 ms 1222.90 ms -0.39 ms
8cb898b+dirty 1209.39 ms 1207.57 ms -1.82 ms

App size

Revision Plain With Sentry Diff
0325426+dirty 3.19 MiB 4.26 MiB 1.07 MiB
0459aee+dirty 3.19 MiB 4.26 MiB 1.07 MiB
3e4cdf5+dirty 3.19 MiB 4.26 MiB 1.07 MiB
2646c98+dirty 3.19 MiB 4.26 MiB 1.07 MiB
e5d5735+dirty 3.19 MiB 4.26 MiB 1.07 MiB
269c976+dirty 3.19 MiB 4.26 MiB 1.07 MiB
8cb898b+dirty 3.19 MiB 4.28 MiB 1.09 MiB

Previous results on branch: antonis/feedback-modal-ui

Startup times

Revision Plain With Sentry Diff
5bd21c5+dirty 1228.94 ms 1220.00 ms -8.94 ms
de197fe+dirty 1207.69 ms 1200.61 ms -7.09 ms
8216878+dirty 1215.94 ms 1208.36 ms -7.57 ms

App size

Revision Plain With Sentry Diff
5bd21c5+dirty 3.19 MiB 4.28 MiB 1.09 MiB
de197fe+dirty 3.19 MiB 4.28 MiB 1.09 MiB
8216878+dirty 3.19 MiB 4.28 MiB 1.09 MiB

@antonis
Copy link
Collaborator Author

antonis commented Jan 31, 2025

The auto-injected form has double the border of the manually created one. That seems to me like a bug.

Good catch 👍
Removed the extra border 05f94f8 and adjusted the height 9ecd8a2 to match the manual

Manually created form Modal
)

Could we make use of the fact we wrap the modal in view and make the wrapper view fade in out the shadow as the modal slides up?

Good idea 👍
Updated with ce1de86

iOS Animation
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-01-31.at.16.52.59.mp4
Android Animation
Screen_recording_20250131_165336.mp4

I've tried the code on iOS and the form does not avoid the on-screen keyboard.

Should be fixed with a7a4e56

Recording
Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2025-01-31.at.19.00.05.mp4

@antonis antonis mentioned this pull request Feb 3, 2025
10 tasks
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