-
Notifications
You must be signed in to change notification settings - Fork 453
Refine message on fenix 2654 #5816
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
base: main
Are you sure you want to change the base?
Refine message on fenix 2654 #5816
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5816 +/- ##
==========================================
- Coverage 85.56% 85.56% -0.01%
==========================================
Files 320 320
Lines 31419 31457 +38
Branches 8649 8659 +10
==========================================
+ Hits 26884 26915 +31
- Misses 4104 4111 +7
Partials 431 431 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
canova
left a comment
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.
Thanks for the PR!
I see that the first paragraph is saying that the profiling requires Firefox desktop, but in the second paragraph we tell them that they can also use the on device profiling. I think it would confuse our users. The old wording from the issue was actually suggested before we supported on device profiling, so it's not up-to-date anymore. See also my suggestions below.
src/components/app/Home.tsx
Outdated
| Recording performance profiles currently requires{' '} | ||
| <a href="https://www.firefox.com/en-US/browsers/desktop/"> | ||
| Firefox for Desktop | ||
| </a> | ||
| . However, existing profiles can be viewed in any modern desktop |
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.
Ah, it looks like we have 2 different instructions now that kinda say the opposite things. Since we have a Localized component below that says it's possible to profile on an Android device, "Recording performance profiles currently requires Firefox for Desktop" is no longer true. Can we move the android on device profiling above, and add the remote profiling instruction below?
For the wording of remote profiling, maybe something like:
You can also profile Firefox for Android remotely from a desktop
computer. For more information, please consult this documentation:{' '}
<a>Profiling Firefox for Android remotely</a>.
| <a>Profiling Firefox for Android directly on device</a>. | ||
| </p> | ||
| </Localized> | ||
| </div> |
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 wording is more suitable for the desktop message, since it says "You can also profile Firefox for Android.". Instead, I would prefer to have a new ftl key with a better wording suited specifically for Firefox for Android.
Maybe something like:
Firefox for Android can be profiled directly on this device. For
more information, please consult this documentation:{' '}
<a>Profiling Firefox for Android directly on device</a>.
Closes #2654.
Before
After