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(feedback): Add theming #4677

Open
wants to merge 14 commits into
base: antonis/4358-Feedback-Form-Autoinject-Button
Choose a base branch
from

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Mar 21, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

Based on #4378

📜 Description

Adds theming options similar to JS

      Sentry.feedbackIntegration({
        colorScheme: 'system',
        themeLight: {
          foreground: '#ff0000',
          background: '#00ff00',
        },
        themeDark: {
          foreground: '#00ff00',
          background: '#ff0000',
        },
      }),
Demo Video

Screen.Recording.2025-03-28.at.3.42.32.PM.mov

Light Dark
Simulator Screenshot - iPhone 16 Pro - 2025-03-27 at 12 03 19 Simulator Screenshot - iPhone 16 Pro - 2025-03-27 at 12 03 43

💡 Motivation and Context

💚 How did you test it?

Sample app

📝 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

Copy link
Contributor

github-actions bot commented Mar 21, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 6c63d03

Copy link
Contributor

github-actions bot commented Mar 21, 2025

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 472.51 ms 472.11 ms -0.40 ms
Size 17.75 MiB 20.13 MiB 2.39 MiB

Baseline results on branch: antonis/4358-Feedback-Form-Autoinject-Button

Startup times

Revision Plain With Sentry Diff
6a56df9 421.08 ms 410.56 ms -10.52 ms
e028035 390.98 ms 404.22 ms 13.25 ms
6c9bb06 432.38 ms 435.92 ms 3.54 ms
36c9278 462.35 ms 443.98 ms -18.37 ms
b383cbc 427.42 ms 427.17 ms -0.25 ms
fab77ad 424.86 ms 423.73 ms -1.13 ms
bc8d4a8 398.20 ms 392.78 ms -5.42 ms
6b05100 459.60 ms 442.80 ms -16.80 ms
4f8eb0b 458.57 ms 467.82 ms 9.24 ms
4a07b14 433.17 ms 449.08 ms 15.91 ms

App size

Revision Plain With Sentry Diff
6a56df9 17.74 MiB 20.11 MiB 2.37 MiB
e028035 17.75 MiB 20.13 MiB 2.38 MiB
6c9bb06 17.75 MiB 20.12 MiB 2.38 MiB
36c9278 17.74 MiB 20.10 MiB 2.37 MiB
b383cbc 17.75 MiB 20.12 MiB 2.37 MiB
fab77ad 17.75 MiB 20.12 MiB 2.37 MiB
bc8d4a8 17.75 MiB 20.12 MiB 2.37 MiB
6b05100 17.74 MiB 20.11 MiB 2.37 MiB
4f8eb0b 17.75 MiB 20.12 MiB 2.37 MiB
4a07b14 17.75 MiB 20.12 MiB 2.38 MiB

Previous results on branch: antonis/feedback-theme

Startup times

Revision Plain With Sentry Diff
6c3f839 427.58 ms 413.10 ms -14.48 ms
cb89989 398.12 ms 393.85 ms -4.27 ms

App size

Revision Plain With Sentry Diff
6c3f839 17.75 MiB 20.13 MiB 2.38 MiB
cb89989 17.75 MiB 20.13 MiB 2.38 MiB

Copy link
Contributor

github-actions bot commented Mar 21, 2025

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 388.04 ms 378.62 ms -9.42 ms
Size 7.15 MiB 8.40 MiB 1.25 MiB

Baseline results on branch: antonis/4358-Feedback-Form-Autoinject-Button

Startup times

Revision Plain With Sentry Diff
b70633e+dirty 368.44 ms 411.94 ms 43.50 ms
6c9bb06+dirty 400.80 ms 429.53 ms 28.73 ms
39fa70e+dirty 411.65 ms 423.04 ms 11.39 ms
1169f78+dirty 380.79 ms 360.83 ms -19.95 ms
6a56df9+dirty 384.17 ms 404.35 ms 20.18 ms
4a07b14+dirty 285.76 ms 300.74 ms 14.98 ms
fab77ad+dirty 394.53 ms 407.42 ms 12.89 ms
6b05100+dirty 390.82 ms 408.63 ms 17.81 ms
e681e44+dirty 396.96 ms 422.58 ms 25.62 ms
e028035+dirty 386.08 ms 389.80 ms 3.72 ms

App size

Revision Plain With Sentry Diff
b70633e+dirty 7.15 MiB 8.38 MiB 1.23 MiB
6c9bb06+dirty 7.15 MiB 8.39 MiB 1.24 MiB
39fa70e+dirty 7.15 MiB 8.40 MiB 1.25 MiB
1169f78+dirty 7.15 MiB 8.39 MiB 1.24 MiB
6a56df9+dirty 7.15 MiB 8.39 MiB 1.24 MiB
4a07b14+dirty 7.15 MiB 8.39 MiB 1.24 MiB
fab77ad+dirty 7.15 MiB 8.39 MiB 1.24 MiB
6b05100+dirty 7.15 MiB 8.38 MiB 1.24 MiB
e681e44+dirty 7.15 MiB 8.40 MiB 1.25 MiB
e028035+dirty 7.15 MiB 8.40 MiB 1.25 MiB

Previous results on branch: antonis/feedback-theme

Startup times

Revision Plain With Sentry Diff
cb89989+dirty 377.51 ms 371.22 ms -6.29 ms
6c3f839+dirty 392.94 ms 409.02 ms 16.08 ms

App size

Revision Plain With Sentry Diff
cb89989+dirty 7.15 MiB 8.40 MiB 1.25 MiB
6c3f839+dirty 7.15 MiB 8.40 MiB 1.25 MiB

Copy link
Contributor

github-actions bot commented Mar 21, 2025

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1211.25 ms 1214.10 ms 2.85 ms
Size 2.63 MiB 3.77 MiB 1.14 MiB

Baseline results on branch: antonis/4358-Feedback-Form-Autoinject-Button

Startup times

Revision Plain With Sentry Diff
2617630+dirty 1232.52 ms 1240.78 ms 8.26 ms
e028035+dirty 1222.20 ms 1224.64 ms 2.44 ms
b383cbc+dirty 1214.57 ms 1220.48 ms 5.91 ms
36c9278+dirty 1227.37 ms 1229.80 ms 2.43 ms
39fa70e+dirty 1225.16 ms 1228.71 ms 3.55 ms
6a56df9+dirty 1227.91 ms 1218.66 ms -9.26 ms
6b05100+dirty 1242.04 ms 1241.65 ms -0.39 ms
4a07b14+dirty 1221.24 ms 1215.02 ms -6.22 ms
1169f78+dirty 1225.43 ms 1225.53 ms 0.10 ms
6c9bb06+dirty 1210.79 ms 1220.06 ms 9.27 ms

App size

Revision Plain With Sentry Diff
2617630+dirty 2.36 MiB 3.13 MiB 783.21 KiB
e028035+dirty 2.63 MiB 3.77 MiB 1.13 MiB
b383cbc+dirty 2.63 MiB 3.75 MiB 1.12 MiB
36c9278+dirty 2.36 MiB 3.13 MiB 784.69 KiB
39fa70e+dirty 2.63 MiB 3.77 MiB 1.13 MiB
6a56df9+dirty 2.36 MiB 3.12 MiB 770.81 KiB
6b05100+dirty 2.36 MiB 3.12 MiB 770.21 KiB
4a07b14+dirty 2.63 MiB 3.71 MiB 1.08 MiB
1169f78+dirty 2.63 MiB 3.76 MiB 1.13 MiB
6c9bb06+dirty 2.63 MiB 3.71 MiB 1.08 MiB

Previous results on branch: antonis/feedback-theme

Startup times

Revision Plain With Sentry Diff
cb89989+dirty 1225.84 ms 1223.33 ms -2.51 ms
6c3f839+dirty 1224.98 ms 1227.81 ms 2.83 ms

App size

Revision Plain With Sentry Diff
cb89989+dirty 2.63 MiB 3.77 MiB 1.13 MiB
6c3f839+dirty 2.63 MiB 3.77 MiB 1.13 MiB

Copy link
Contributor

github-actions bot commented Mar 21, 2025

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1215.82 ms 1227.36 ms 11.54 ms
Size 3.19 MiB 4.34 MiB 1.15 MiB

Baseline results on branch: antonis/4358-Feedback-Form-Autoinject-Button

Startup times

Revision Plain With Sentry Diff
2617630+dirty 1236.02 ms 1243.86 ms 7.84 ms
e028035+dirty 1216.18 ms 1212.04 ms -4.14 ms
b383cbc+dirty 1240.73 ms 1257.34 ms 16.61 ms
36c9278+dirty 1238.84 ms 1239.52 ms 0.68 ms
39fa70e+dirty 1228.13 ms 1232.24 ms 4.12 ms
6a56df9+dirty 1231.62 ms 1244.02 ms 12.40 ms
6b05100+dirty 1245.56 ms 1240.36 ms -5.20 ms
4a07b14+dirty 1213.45 ms 1206.12 ms -7.32 ms
1169f78+dirty 1234.33 ms 1230.53 ms -3.80 ms
6c9bb06+dirty 1235.22 ms 1226.68 ms -8.54 ms

App size

Revision Plain With Sentry Diff
2617630+dirty 2.92 MiB 3.69 MiB 794.45 KiB
e028035+dirty 3.19 MiB 4.33 MiB 1.14 MiB
b383cbc+dirty 3.19 MiB 4.32 MiB 1.13 MiB
36c9278+dirty 2.92 MiB 3.70 MiB 796.02 KiB
39fa70e+dirty 3.19 MiB 4.33 MiB 1.14 MiB
6a56df9+dirty 2.92 MiB 3.68 MiB 783.36 KiB
6b05100+dirty 2.92 MiB 3.68 MiB 782.77 KiB
4a07b14+dirty 3.19 MiB 4.28 MiB 1.09 MiB
1169f78+dirty 3.19 MiB 4.32 MiB 1.14 MiB
6c9bb06+dirty 3.19 MiB 4.28 MiB 1.09 MiB

Previous results on branch: antonis/feedback-theme

Startup times

Revision Plain With Sentry Diff
cb89989+dirty 1228.14 ms 1233.66 ms 5.52 ms
6c3f839+dirty 1229.57 ms 1231.72 ms 2.15 ms

App size

Revision Plain With Sentry Diff
cb89989+dirty 3.19 MiB 4.33 MiB 1.15 MiB
6c3f839+dirty 3.19 MiB 4.33 MiB 1.15 MiB

@antonis antonis marked this pull request as ready for review March 24, 2025 17:32
/**
* Get the theme for the feedback widget based on the current color scheme
*/
export function getTheme(): FeedbackWidgetTheme {
Copy link
Member

Choose a reason for hiding this comment

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

The get theme is being called when the styles consts are created, which is before Sentry.init, and thus, the feedbackIntegration theme options are ignored.

I assume this might be solved in the reactive follow up, but it seems like a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the feedback @krystofwoldrich 🙇

I wasn't able to reproduce with the sample app in dev mode but I agree that this is a bug and the code needs restructuring. I'll iterate on this asap.

Recording

Screen.Recording.2025-03-27.at.12.48.50.PM.mov

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for giving it a try. It might have been my machine issue, I'try it again.

Copy link
Collaborator Author

@antonis antonis Mar 28, 2025

Choose a reason for hiding this comment

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

I've fixed this along with adding dynamic theming support with 7a331b6

(also added a demo video at the PR description)

@krystofwoldrich
Copy link
Member

Overall, it's looking good, but I've noticed some issues with customizing the theme.

We should add tests to ensure that the correct values are applied when using custom styles and custom themes.

@antonis
Copy link
Collaborator Author

antonis commented Mar 28, 2025

Thank you for all your feedback @krystofwoldrich 🙇

This should be ready for another go. Note that I now also cover the dynamic theme changes on this PR with 7a331b6 as part of the refactoring discussed above.

We should add tests to ensure that the correct values are applied when using custom styles and custom themes.

I've added snapshot tests to cover this with 6c63d03

@antonis antonis requested a review from krystofwoldrich March 28, 2025 14:13
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