Skip to content
This repository has been archived by the owner on Jan 31, 2018. It is now read-only.

[bug 969667] Merge desktop and mobile forms #221

Merged
merged 3 commits into from Feb 26, 2014
Merged

[bug 969667] Merge desktop and mobile forms #221

merged 3 commits into from Feb 26, 2014

Conversation

willkg
Copy link
Member

@willkg willkg commented Feb 19, 2014

This is a work in progress. It sort of works, but has a bunch of issues that I can't figure out. To access it, go to http://127.0.0.1/en-US/feedback/generic .

Problems:

  1. On big screens, I find the text hard to read and cluttered. It's also all bunched up at the top in the middle. Media queries can probably help fix this, but my media query fu sux0rz.
  2. The browser's back button goes back a page in history--it doesn't go back a card. That's not a big deal with a mobile browser where you can see the web page back button, but hugely problematic on desktop where it's really far away from the form and it's very likely someone will try to use their browser back button. I don't know how to fix this offhand, but I think I have to use push state (or whatever it's called).

Beyond that, I think the privacy messaging and the labeling is better. This includes the url field that the mobile form doesn't have currently (win!) and it's easier to read than the desktop form (win!).

Also, I'm creating new .css and .js files and duplicating some things right now. I'll fix some of the duplication (duplicate images, duplicating in .less and .css files between this new form and the Firefox OS form, etc) in a later refactoring pass.

Anyhow, work in progress.

Feedback?

</a></h1>

{% block body %}
<nav id="nav-access">
Copy link
Contributor

Choose a reason for hiding this comment

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

yay for <nav/>!

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, it's the same as before :)

Copy link
Contributor

Choose a reason for hiding this comment

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

(I wish github did a smarter diff when just indentation changes)

@willkg
Copy link
Member Author

willkg commented Feb 24, 2014

^^^ That fixes problem 2 regarding the browser's back button.

This is still a WIP--there are outstanding things that need fixing first.

@willkg
Copy link
Member Author

willkg commented Feb 24, 2014

^^^ That rebases against master and then fixes some display issues. This looks good to me on my phone and looks decent enough to me on desktop.

I also cleaned up some weird CSS, JS and HTML, so theoretically this is good to go now.

r?

</label>
<label class="sad" for="description">
{{ _('Please describe your problem below.') }}
{{ _('Please be as specific as you can.') }}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is goofy and we should combine these two strings, however, if I do that, then this is a new string and then I'm blocked from landing it for a while. So I left it goofy for now.

@willkg
Copy link
Member Author

willkg commented Feb 24, 2014

One important thing to test here is that I didn't introduce any new strings. You should be able to get the latest strings, compile them and then go through the whole form in some locale that's fully translated:

      it: ================================================== 100
      cs: ================================================== 100
      es: ================================================== 100
      nl: ================================================== 100
      tr: ================================================== 100
      lt: ================================================== 100
      pl: ================================================== 100
      fr: ================================================== 100
   pt_BR: ================================================== 100
      hr: ================================================== 100
      fy: ================================================== 100
   zh_TW: ================================================== 100
      hu: ================================================== 100
      ja: ================================================== 100
      sr: ================================================== 100
      sq: ================================================== 100
      ko: ================================================== 100
      sk: ================================================== 100
      de: ================================================== 100
   nb_NO: ================================================== 100

@rlr
Copy link
Contributor

rlr commented Feb 24, 2014

OK, so what's the next step? This looks very similar to the existing mobile form but very different from the desktop version. I don't see any "responsiveness"?

It all works based on my testing. I do think it will need CSS love, whether it be a brand new design or applying the old style to this form when viewed in bigger screen sizes.

* create new generic form that merges desktop and mobile forms
* move around instructions so important instructions come before the
  fields they apply to--this is better for privacy-related information
* add pushstate support so going back and forth in the form works with
  form controls and browser controls
* this doesn't support jumping into the middle of the form
* rework infrastructure so it's possible to add additional cards and
  measure drop-off rates
* makes this structurally like the firefox os feedback form so all forms
  now have a similar structure

This looks a little worse on Desktop, though. This form could definitely
use better strings and better design, but that's out of the scope of
this change and would require a lot more work.
@willkg
Copy link
Member Author

willkg commented Feb 24, 2014

^^^ That rebases all the changes into one with a better commit message.

I still need to look at the Thank You page and make sure you get to the right one depending on whether you're using desktop or mobile because we're not changing that in this round.

@willkg
Copy link
Member Author

willkg commented Feb 25, 2014

@rlr I can't reproduce the problem with the thank you page you're seeing. The thanks view is marked with a @mobile_template decorator and I see the desktop thanks (which looks awful) with desktop and the mobile thanks (which looks a little better) on my phone.

How'd you end up seeing the desktop thanks page on a device that shouldn't be seeing it?

@rlr
Copy link
Contributor

rlr commented Feb 25, 2014

oh, I see. I did the old android form from desktop browser. carry on

* nixes desktop and mobile form routing (yay!)
* replaces that with new generic form
* update tests to denote that we now essentially ignore validation
  errors with email addresses and descriptions--Thanks!
* update ancient android form post handling to also ignore errors
* clean up code after all that
This cleans up some of the CSS and JS after removing the desktop and
mobile feedback forms.
@willkg
Copy link
Member Author

willkg commented Feb 25, 2014

^^^ Matt gave me the ok, so I nixed the desktop and mobile forms and related bits.

I probably missed some things, but I'm not seeing any errors, so it's probably mostly right.

Quick r?

@rlr
Copy link
Contributor

rlr commented Feb 25, 2014

The old URLs /feedback/firefox.desktop.stable/ and /feedback/firefox.android.stable/ no longer work. Is that ok? They aren't linked from anywhere?

@rlr
Copy link
Contributor

rlr commented Feb 25, 2014

or /feedback/firefox.fxos.stable/ for that matter. I thought those URLs would still work but just use the generic form. Or am I lost 😕

@rlr
Copy link
Contributor

rlr commented Feb 25, 2014

Ah, I see. You can do things like /feedback/firefox/ and /feedback/android/26.0/stable/. I had forgot about that...

But you still dropped support for certain URLs, I'm cool with that if you are but I don't know if that breaks anything out in the wild.

@rlr
Copy link
Contributor

rlr commented Feb 25, 2014

I tested the form with different user agents and it all WFM.

So, I think this is good to go assuming it's OK for those URLs to no longer work.

@willkg
Copy link
Member Author

willkg commented Feb 26, 2014

Those feedback form urls were created solely for testing. If someone was using them in the wild, I would be:

  1. surprised
  2. annoyed

When a person using those old urls shows up, I will waggle my finger at them.

We have product urls. It's kind of irritating now since they're db-driven and not written down somewhere, but it'll get easier when I add a product picker.

Firefox OS is a special case--we have a special form that asks additional questions, so all Firefox OS things get shunted to that form. At some point we might want to merge that one with this one, but not in this pass. Maybe it makes sense to generalize it so that we can specify which cards each product gets and generate the cards automatically. I'm not thinking that far ahead right now, though.

@rlr
Copy link
Contributor

rlr commented Feb 26, 2014

b:smiley:d

@willkg willkg merged commit cfc3814 into mozilla:master Feb 26, 2014
@willkg
Copy link
Member Author

willkg commented Feb 26, 2014

Landed in master in:

cfc3814 [bug 969667] Clean up CSS/JS
600f02c [bug 969667] Replace mobile/desktop forms with generic
e708164 [bug 969667] Merge desktop and mobile forms

@willkg willkg deleted the 969667-merge-forms branch March 14, 2014 19:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants