firefox-devtools / profiler

Firefox Profiler — Web app for Firefox performance analysis
https://profiler.firefox.com
Mozilla Public License 2.0
1.18k stars 385 forks source link

Detect Fenix to display a better message in the home page #2654

Open julienw opened 4 years ago

julienw commented 4 years ago

see https://github.com/webcompat/web-bugs/issues/55477 https://github.com/mozilla-mobile/fenix/issues/12635.

┆Issue is synchronized with this Jira Task

julienw commented 3 years ago

The goal here is to display a specific message to Firefox for Android.

  1. Add a new state 'firefox-android' to https://github.com/firefox-devtools/profiler/blob/8a22ab802964988777a55be4bff2197dbec4bdf2/src/components/app/Home.js#L236-L244
  2. Add a isMobile function at the bottom, near isFirefox. Probably we can check for /\Android\b/. (see all Firefox user agents here).
  3. In the constructor, use this new isMobile function in https://github.com/firefox-devtools/profiler/blob/8a22ab802964988777a55be4bff2197dbec4bdf2/src/components/app/Home.js#L252-L257 to set the state to 'firefox-android'
  4. Add a new case in https://github.com/firefox-devtools/profiler/blob/8a22ab802964988777a55be4bff2197dbec4bdf2/src/components/app/Home.js#L301-L320 calling a new method renderFirefoxAndroidInstructions
  5. This new method can look a lot like renderOtherBrowserInstructions, with the following text:

    
              Recording performance profiles currently requires{' '}
              <a href="https://www.mozilla.org/en-US/firefox/new/">Firefox for Desktop</a>.
              However, existing profiles can be viewed in any modern desktop browser.
Anita-ihuman commented 3 years ago

I would like to tackle this issue, can I get a headstart?

julienw commented 3 years ago

Hey @Anita-ihuman, you can work on this. I just want to double-check that you're here for the Outreachy contribution period, is that right? Thanks for your confirmation.

julienw commented 3 years ago

Hey @Anita-ihuman, are you still working on this? Do you have any question?

Anita-ihuman commented 3 years ago

Yeah, I am working on it.

Anita-ihuman commented 3 years ago

I have made the changes but I seam to be having errors with he testing.

julienw commented 3 years ago

hey @Anita-ihuman, do you still intend to look at this more? Where can we see your changes so that we can give a feedback? It's totally OK if you don't want to work on this more, but please tell us so that we can update the status of this bug and somebody can take it :-) Thanks!

julienw commented 3 years ago

Unassigning for inactivity. Thanks!

NKwatra commented 3 years ago

Hey @julienw, I am not sure about it, but shouldn't regex for isMobile function be /Mobi|Tablet\b/? Based on below content from Firefox user agents page

"if you use UA sniffing to target content to a device form factor, please look for Mobi (to include Opera Mobile, which uses "Mobi") for the phone form factor and do not assume any correlation between "Android" and the device form factor. This way, your code will work if/when Firefox ships on other phone/tablet operating systems or Android is used for laptops."

The above will work from Firefox 11 onwards and will cover Mobiles and Tablets running any OS.

julienw commented 3 years ago

Hey, thanks for your comment! Your suggestion is valid in the general case, where we'd want to target mobile devices in general. But in this case we want to specifically detect firefox on Android so that we can display specific instructions. Maybe the new function should be called "isAndroid" though?

NKwatra commented 3 years ago

I guess, "isAndroid" would be a better name in that case. Can I work on this then?

julienw commented 3 years ago

Yes sure. Please note that during the holiday season we will be less responsive than usually.

yaduu572 commented 3 years ago

Hello, is this issue still open? I would like to work on it.

julienw commented 3 years ago

hey @yaduu572 , no it's not, @NKwatra is working on this (I still have to look at that PR, sorry 🙏). I forgot to set the flags accordingly, sorry about that!

yaduu572 commented 3 years ago

hey @yaduu572 , no it's not, @NKwatra is working on this (I still have to look at that PR, sorry 🙏). I forgot to set the flags accordingly, sorry about that!

yeah no problem.. Can you please tell me if there any good first issues left in this project? I would like to work in this project.

julienw commented 3 years ago

@yaduu572 It's now available actually, if you still want to. You can have a look at the existing PR #3117 and the comments there as a quick startup.

yaduu572 commented 3 years ago

@yaduu572 It's now available actually, if you still want to. You can have a look at the existing PR #3117 and the comments there as a quick startup.

Yeah sure please guide me as I am new to open source. Thanks

julienw commented 3 years ago

I won't be able to guide you really, but you can start by looking at https://github.com/firefox-devtools/profiler/blob/main/CONTRIBUTING.md. Thanks

yaduu572 commented 3 years ago

I won't be able to guide you really, but you can start by looking at https://github.com/firefox-devtools/profiler/blob/main/CONTRIBUTING.md. Thanks

Yeah sure thanks

ritikBhandari commented 2 years ago

Hi!! Is this issue still open? I'd like to work on this!

julienw commented 2 years ago

sure @ritikBhandari !

ritikBhandari commented 2 years ago

Hi @julienw,

Though I'm done with the issue, I had a doubt about renderFirefoxAndroidInstructions function.

This new method can look a lot like renderOtherBrowserInstructions, with the following text:

Can u pls claritfy what needs to be done exactly in the function. Is it just the change in the message. Something like:

- <a href="https://www.mozilla.org/en US/firefox/new/">Firefox</a>
+ <a href="https://www.mozilla.org/en-US/firefox/new/">Firefox for Desktop</a>

Is that it?

julienw commented 2 years ago

Yeah I believe this is it. If you want some more feedback, the easiest would be to open a pull request, that will automatically build a deploy preview that we can try out :-)

Thanks again!

alpacino767 commented 5 months ago

Hello @julienw Is this issue still open? If yes please can I be assigned to it as I'd like to work on it.