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

Fixes #821: Add Tablet info to browser name #941

Merged
merged 3 commits into from Mar 9, 2016

Conversation

dshgna
Copy link
Contributor

@dshgna dshgna commented Mar 4, 2016

Initial commit.

@dshgna
Copy link
Contributor Author

dshgna commented Mar 4, 2016

r? @miketaylr Made an initial commit to verify that if I'm moving in the right direction.

@miketaylr
Copy link
Member

Looks like you're on the right track, yes!

It might be good to add a few tests to https://github.com/webcompat/webcompat.com/blob/master/tests/test_helpers.py to make sure the right UA strings get the Tablet treatment and some non-Tablet ones don't.

@dshgna
Copy link
Contributor Author

dshgna commented Mar 5, 2016

r? @miketaylr I'm a bit confused on moving forwards. As the browser name has been set to include tablets, what else should be changed in the backend logic? Could you give me some hints please? :)

@karlcow
Copy link
Member

karlcow commented Mar 8, 2016

hehe @miketaylr ping.

But to help. The science of UA strings is black magic. So basically there's no perfection. But we want to make sure that indeed we get the right label when there is a tablet hitting webcompat.

So currently get_browser_name is used in
https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/views.py#L129-L133

which calls itself the templating engine for the index page and prepopulate the form:

capture d ecran 2016-03-08 a 12 39 13

<div class="wc-Form-group js-Form-group">
            <label class="wc-Form-label" for="browser">Browser / Version</label> <input class="wc-Form-item" id="browser" name="browser" value="Firefox 47.0" type="text">
          </div>

When we actually submit the form we get the value of the UA from

    form['ua_header'] = request.headers.get('User-Agent')

https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/views.py#L162

Then we head to report_issue

https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/issues.py#L19

which is using build_formdata
https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/form.py#L139

which is reading the browser parameter
https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/form.py#L181

but also triggers the labels:
https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/form.py#L178

aka get_labels()
https://github.com/dshgna/webcompat.com/blob/issue/821/webcompat/form.py#L106

SO I guess the final question is what type of labels do we want in the bug reporting system when there is a tablet :)

@miketaylr
Copy link
Member

hehe @miketaylr ping.

Oh! So sorry @dshgna, this mention escaped me. 🙈

@miketaylr
Copy link
Member

@dshgna if you click on the Details link of the travis-ci build (see https://cloudup.com/cy5FSvXC0WB), you'll see the following error (scroll down and look for the red text):

The command "pep8 --ignore=E402 webcompat/ tests/ config/secrets.py.example" failed and exited with 1 during .

Can you re-run pep8 and fix any errors there (and push the fixes)?

@miketaylr
Copy link
Member

@karlcow do you think it makes sense to have "firefox mobile tablet" label in addition to "firefox mobile"? Or is the name in the bug report "Firefox Mobile (Tablet)" sufficient? I don't have any strong feelings, I'm sort of leaning towards just one "firefox mobile" label...

@miketaylr
Copy link
Member

@dshgna a few commit nits to take care of if you don't mind:

When you fix up pep8 errors, can you add "Issue #821. " to the beginning of the commit message (for the test commit)? Likewise, for the first commit -- can you add a hash mark in front of the issue number, e.g., "Fixes #821"?

That will help link all the commits to the issue. If you need help on how to re-write a commit message just let me know!

@karlcow
Copy link
Member

karlcow commented Mar 9, 2016

@miketaylr about

@karlcow do you think it makes sense to have "firefox mobile tablet" label in addition to "firefox mobile"?

I would rather have two differents labels such as:

  • browsername-tablet
  • browsername-mobile

and/or an information about the screen size. The reason it has happen often that I didn't know if the bug reporter was talking about tablets and was testing on a small viewport device. In some cases, Web sites choose to send desktop sites to the tablet form factor.

I suspect you guess the form factor by looking at the UA in general and because you spent most of your time in github instead of webcompat ;) :p (Use the force, mike, it helps to improve the product hmm no laser sabre in emojis. Maybe this will do ⚡️🗡🐸)

@miketaylr
Copy link
Member

I would rather have two differents labels such as:

OK, that's what this should do. 👍

and/or an information about the screen size. The reason it has happen often that I didn't know if the bug reporter was talking about tablets and was testing on a small viewport device. In some cases, Web sites choose to send desktop sites to the tablet form factor.

Sounds like a good feature request. File a bug? This would be good info to put in the bug body.

I suspect you guess the form factor by looking at the UA in general and because you spent most of your time in github instead of webcompat ;)

Yes and no. :p

miketaylr pushed a commit that referenced this pull request Mar 9, 2016
Fixes #821: Add Tablet info to browser name
@miketaylr miketaylr merged commit 2dccd7a into webcompat:master Mar 9, 2016
@dshgna
Copy link
Contributor Author

dshgna commented Mar 9, 2016

@miketaylr Just a bit confused here on the merge :)

I would rather have two different labels such as:

I got the understanding that I had to change the 'browser mobile tablet' to 'browser tablet' and just made a commit as such.

So, is this okay as is?

@miketaylr
Copy link
Member

Personally I think "firefox mobile tablet" is ok! Let's see how it feels and we can change it up as needed.

@dshgna
Copy link
Contributor Author

dshgna commented Mar 9, 2016

@miketaylr sure thanks! 👍

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.

None yet

3 participants