-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
(1) feat: Add Feedback From Component #4328
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Krystof Woldrich <[email protected]>
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9433f35 | 347.64 ms | 356.22 ms | 8.58 ms |
ddc0552 | 472.92 ms | 460.66 ms | -12.26 ms |
f54118b | 441.24 ms | 431.92 ms | -9.32 ms |
4297324 | 536.61 ms | 542.48 ms | 5.87 ms |
cdf2f33 | 469.46 ms | 462.17 ms | -7.29 ms |
9f0f6c8 | 433.91 ms | 429.16 ms | -4.75 ms |
3ffcddd | 302.92 ms | 315.80 ms | 12.88 ms |
1c65324 | 426.37 ms | 460.36 ms | 33.99 ms |
416f465 | 446.96 ms | 454.22 ms | 7.26 ms |
e73d82f | 475.82 ms | 506.55 ms | 30.73 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9433f35 | 17.73 MiB | 19.81 MiB | 2.08 MiB |
ddc0552 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
f54118b | 17.74 MiB | 20.08 MiB | 2.35 MiB |
4297324 | 17.74 MiB | 20.08 MiB | 2.34 MiB |
cdf2f33 | 17.74 MiB | 20.08 MiB | 2.34 MiB |
9f0f6c8 | 17.74 MiB | 20.08 MiB | 2.35 MiB |
3ffcddd | 17.73 MiB | 19.75 MiB | 2.02 MiB |
1c65324 | 17.73 MiB | 19.95 MiB | 2.21 MiB |
416f465 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
e73d82f | 17.73 MiB | 20.07 MiB | 2.33 MiB |
Previous results on branch: antonis/3859-newCaptureFeedbackAPI-Form
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
27e1bf3 | 463.19 ms | 478.80 ms | 15.61 ms |
0781f75 | 452.32 ms | 457.22 ms | 4.91 ms |
a3ba405 | 438.16 ms | 435.78 ms | -2.38 ms |
a06f6ba | 424.02 ms | 415.82 ms | -8.20 ms |
d33790a | 442.93 ms | 439.94 ms | -3.00 ms |
561640f | 461.96 ms | 458.11 ms | -3.85 ms |
cadf235 | 462.20 ms | 463.34 ms | 1.14 ms |
26fc306 | 426.80 ms | 421.58 ms | -5.22 ms |
50c70c0 | 496.82 ms | 526.02 ms | 29.20 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
27e1bf3 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
0781f75 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
a3ba405 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
a06f6ba | 17.74 MiB | 20.09 MiB | 2.35 MiB |
d33790a | 17.74 MiB | 20.10 MiB | 2.36 MiB |
561640f | 17.74 MiB | 20.09 MiB | 2.35 MiB |
cadf235 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
26fc306 | 17.74 MiB | 20.09 MiB | 2.35 MiB |
50c70c0 | 17.74 MiB | 20.10 MiB | 2.36 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4297324+dirty | 385.33 ms | 435.68 ms | 50.35 ms |
9282172+dirty | 363.57 ms | 399.78 ms | 36.20 ms |
e1ea4a8+dirty | 451.98 ms | 497.58 ms | 45.60 ms |
e540498+dirty | 408.56 ms | 480.00 ms | 71.44 ms |
db44eaf+dirty | 394.26 ms | 438.88 ms | 44.62 ms |
70e6261+dirty | 395.08 ms | 408.12 ms | 13.04 ms |
0db0c72+dirty | 335.20 ms | 351.06 ms | 15.86 ms |
457e29f+dirty | 591.49 ms | 612.96 ms | 21.47 ms |
9433f35+dirty | 265.50 ms | 336.08 ms | 70.58 ms |
61310e1+dirty | 463.51 ms | 511.09 ms | 47.57 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
4297324+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
9282172+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
e1ea4a8+dirty | 7.15 MiB | 8.35 MiB | 1.20 MiB |
e540498+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
db44eaf+dirty | 7.15 MiB | 8.36 MiB | 1.21 MiB |
70e6261+dirty | 7.15 MiB | 8.21 MiB | 1.07 MiB |
0db0c72+dirty | 7.15 MiB | 8.04 MiB | 911.02 KiB |
457e29f+dirty | 7.15 MiB | 8.10 MiB | 981.29 KiB |
9433f35+dirty | 7.15 MiB | 8.08 MiB | 959.34 KiB |
61310e1+dirty | 7.15 MiB | 8.36 MiB | 1.21 MiB |
Previous results on branch: antonis/3859-newCaptureFeedbackAPI-Form
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 359.67 ms | 436.86 ms | 77.19 ms |
26fc306+dirty | 382.83 ms | 435.31 ms | 52.48 ms |
0781f75+dirty | 406.72 ms | 454.80 ms | 48.08 ms |
a06f6ba+dirty | 381.50 ms | 429.77 ms | 48.27 ms |
50c70c0+dirty | 385.30 ms | 433.06 ms | 47.76 ms |
561640f+dirty | 378.73 ms | 442.25 ms | 63.52 ms |
27e1bf3+dirty | 398.69 ms | 439.39 ms | 40.69 ms |
cadf235+dirty | 455.51 ms | 451.64 ms | -3.87 ms |
d33790a+dirty | 404.87 ms | 473.06 ms | 68.19 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
26fc306+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
0781f75+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
a06f6ba+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
50c70c0+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
561640f+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
27e1bf3+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
cadf235+dirty | 7.15 MiB | 8.37 MiB | 1.22 MiB |
d33790a+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 1253.39 ms | 1256.30 ms | 2.91 ms |
3853f43+dirty | 1271.74 ms | 1278.04 ms | 6.30 ms |
7fd512a+dirty | 1239.41 ms | 1241.50 ms | 2.09 ms |
457e29f+dirty | 1256.71 ms | 1258.50 ms | 1.79 ms |
ac41368+dirty | 1226.69 ms | 1229.96 ms | 3.27 ms |
9cd0e9f+dirty | 1244.61 ms | 1247.43 ms | 2.82 ms |
9282172+dirty | 1227.60 ms | 1232.69 ms | 5.09 ms |
d0bf494+dirty | 1266.20 ms | 1267.52 ms | 1.32 ms |
f06c879+dirty | 1285.14 ms | 1285.86 ms | 0.72 ms |
5a22220+dirty | 1246.18 ms | 1249.61 ms | 3.43 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 2.92 MiB | 3.41 MiB | 499.97 KiB |
3853f43+dirty | 2.92 MiB | 3.41 MiB | 503.54 KiB |
7fd512a+dirty | 2.92 MiB | 3.66 MiB | 758.62 KiB |
457e29f+dirty | 2.92 MiB | 3.43 MiB | 524.75 KiB |
ac41368+dirty | 2.92 MiB | 3.69 MiB | 794.29 KiB |
9cd0e9f+dirty | 2.92 MiB | 3.64 MiB | 741.23 KiB |
9282172+dirty | 2.92 MiB | 3.67 MiB | 772.46 KiB |
d0bf494+dirty | 2.92 MiB | 3.40 MiB | 488.08 KiB |
f06c879+dirty | 2.92 MiB | 3.44 MiB | 533.24 KiB |
5a22220+dirty | 2.92 MiB | 3.48 MiB | 575.81 KiB |
Previous results on branch: antonis/3859-newCaptureFeedbackAPI-Form
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 1229.31 ms | 1228.16 ms | -1.14 ms |
a06f6ba+dirty | 1235.31 ms | 1238.76 ms | 3.45 ms |
50c70c0+dirty | 1226.61 ms | 1225.02 ms | -1.59 ms |
d33790a+dirty | 1247.14 ms | 1242.86 ms | -4.28 ms |
cadf235+dirty | 1225.19 ms | 1231.65 ms | 6.47 ms |
27e1bf3+dirty | 1245.78 ms | 1244.38 ms | -1.40 ms |
0781f75+dirty | 1247.90 ms | 1237.11 ms | -10.79 ms |
26fc306+dirty | 1229.10 ms | 1227.88 ms | -1.22 ms |
561640f+dirty | 1237.10 ms | 1229.59 ms | -7.51 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 2.92 MiB | 3.67 MiB | 773.65 KiB |
a06f6ba+dirty | 2.92 MiB | 3.67 MiB | 773.87 KiB |
50c70c0+dirty | 2.92 MiB | 3.67 MiB | 773.48 KiB |
d33790a+dirty | 2.92 MiB | 3.67 MiB | 773.59 KiB |
cadf235+dirty | 2.92 MiB | 3.67 MiB | 773.97 KiB |
27e1bf3+dirty | 2.92 MiB | 3.67 MiB | 773.54 KiB |
0781f75+dirty | 2.92 MiB | 3.67 MiB | 773.83 KiB |
26fc306+dirty | 2.92 MiB | 3.67 MiB | 773.77 KiB |
561640f+dirty | 2.92 MiB | 3.67 MiB | 773.72 KiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 1246.96 ms | 1255.73 ms | 8.77 ms |
3853f43+dirty | 1221.82 ms | 1242.64 ms | 20.82 ms |
7fd512a+dirty | 1218.78 ms | 1217.25 ms | -1.53 ms |
457e29f+dirty | 1253.94 ms | 1269.18 ms | 15.24 ms |
ac41368+dirty | 1226.65 ms | 1237.90 ms | 11.24 ms |
9cd0e9f+dirty | 1224.94 ms | 1239.88 ms | 14.94 ms |
9282172+dirty | 1237.27 ms | 1242.20 ms | 4.94 ms |
d0bf494+dirty | 1289.40 ms | 1298.40 ms | 9.00 ms |
f06c879+dirty | 1252.64 ms | 1259.66 ms | 7.02 ms |
5a22220+dirty | 1209.49 ms | 1220.94 ms | 11.45 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9c48b2c+dirty | 2.36 MiB | 2.85 MiB | 495.77 KiB |
3853f43+dirty | 2.36 MiB | 2.85 MiB | 499.81 KiB |
7fd512a+dirty | 2.36 MiB | 3.10 MiB | 753.35 KiB |
457e29f+dirty | 2.36 MiB | 2.87 MiB | 520.67 KiB |
ac41368+dirty | 2.36 MiB | 3.14 MiB | 793.46 KiB |
9cd0e9f+dirty | 2.36 MiB | 3.08 MiB | 735.56 KiB |
9282172+dirty | 2.36 MiB | 3.11 MiB | 759.89 KiB |
d0bf494+dirty | 2.36 MiB | 2.83 MiB | 481.15 KiB |
f06c879+dirty | 2.36 MiB | 2.88 MiB | 530.42 KiB |
5a22220+dirty | 2.36 MiB | 2.92 MiB | 570.21 KiB |
Previous results on branch: antonis/3859-newCaptureFeedbackAPI-Form
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 1223.00 ms | 1219.06 ms | -3.94 ms |
a06f6ba+dirty | 1230.45 ms | 1227.09 ms | -3.36 ms |
50c70c0+dirty | 1228.06 ms | 1224.43 ms | -3.64 ms |
d33790a+dirty | 1234.19 ms | 1231.76 ms | -2.43 ms |
cadf235+dirty | 1223.89 ms | 1236.22 ms | 12.33 ms |
27e1bf3+dirty | 1230.92 ms | 1232.33 ms | 1.41 ms |
0781f75+dirty | 1222.19 ms | 1222.11 ms | -0.08 ms |
26fc306+dirty | 1227.25 ms | 1225.85 ms | -1.40 ms |
561640f+dirty | 1220.45 ms | 1227.02 ms | 6.57 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a3ba405+dirty | 2.36 MiB | 3.11 MiB | 760.99 KiB |
a06f6ba+dirty | 2.36 MiB | 3.11 MiB | 761.35 KiB |
50c70c0+dirty | 2.36 MiB | 3.11 MiB | 760.92 KiB |
d33790a+dirty | 2.36 MiB | 3.11 MiB | 761.06 KiB |
cadf235+dirty | 2.36 MiB | 3.11 MiB | 761.47 KiB |
27e1bf3+dirty | 2.36 MiB | 3.11 MiB | 761.03 KiB |
0781f75+dirty | 2.36 MiB | 3.11 MiB | 761.35 KiB |
26fc306+dirty | 2.36 MiB | 3.11 MiB | 761.18 KiB |
561640f+dirty | 2.36 MiB | 3.11 MiB | 761.19 KiB |
@@ -0,0 +1,54 @@ | |||
import type { FeedbackFormStyles } from './FeedbackForm.types'; | |||
|
|||
const defaultStyles: FeedbackFormStyles = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's match the default with JS.
Mainly I've noticed submitButton backgroundColor (JS has rgba(88, 74, 192, 1)
) and test color #2b2233
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to align with the light theme of JS with 30a7b10
Can you add a snapshot test for the form? One for defaults and maybe one for custom styles and one for custom "content" (meaning custom labels, text and placeholders) |
submitText: { | ||
color: '#fff', | ||
fontSize: 18, | ||
fontWeight: 'bold', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a personal preference, but I would make both the button bold or both regular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with 3aacaf7 to use regular fonts weight.
Co-authored-by: Krystof Woldrich <[email protected]>
Co-authored-by: Krystof Woldrich <[email protected]>
Co-authored-by: Krystof Woldrich <[email protected]>
…hub.com/getsentry/sentry-react-native into antonis/3859-newCaptureFeedbackAPI-Form
email: getCurrentScope().getUser().email || '', | ||
name: getCurrentScope().getUser().name || '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's read the current user when rendering the feedback form.
Loading the email and name here could result in incorrect values. As this will be evaluated during the file import.
Let's also check it in the test that the getUser was called after render but not before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch 👍
Updated with 57d99e9
return; | ||
} | ||
|
||
if ((config.isEmailRequired || trimmedEmail.length > 0) && !this._isValidEmail(trimmedEmail)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using custom regex for the email validation, let's add an option to enable/disable the validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an option with c45a5e6
Good catch @krystofwoldrich 🙇 |
const { onFormClose } = { ...defaultConfiguration, ...this.props }; | ||
const config: FeedbackGeneralConfiguration = { ...defaultConfiguration, ...this.props }; | ||
const text: FeedbackTextConfiguration = { ...defaultConfiguration, ...this.props }; | ||
const styles: FeedbackFormStyles = { ...defaultStyles, ...this.props.styles }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let do the merge only once { ...defaultConfiguration, ...this.props }
.
On moder devices it should not make a perf difference, but let's be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 👍
Updated with 6e39119
That's a good idea. Thank you for the suggestion and the linked resource 🙇 |
📢 Type of change
📜 Description
Adds a basic feedback form with the following fields: name, email, description.
The form text and styles can be customised.
💡 Motivation and Context
Fixes #4336
💚 How did you test it?
CI, Manual testing (example)
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps