fix(styles): make setting header UI better #3089

johngruen · 2015-09-21T19:53:48Z

@pdehaan @zaach maybe give this a review? I added some stuff into the settings view to twiddle the DOM when a user adds or removes a display name.

Fixes #3055

johngruen · 2015-09-21T19:54:11Z

Fixes #3055

pdehaan · 2015-09-21T20:56:16Z

app/styles/_variables.scss

@@ -35,6 +35,7 @@ $input-background-disabled-color: #f2f2f2;
 $input-border-disabled-color: #dddddd;
 $input-disabled-color: #64778c;
 $secondary-button-border-color: #c1c1c1;
+$black: #fff;


nit: this is a bit confusing. Since $black is technically white.

should this be $modal-border-color?

@pdehaan that IS confusing

vladikoff · 2015-09-22T01:08:43Z

app/scripts/views/settings.js

+      var displayName = account.get('displayName');
+      var email = account.get('email');
+
+      var $cardHeader = $('.card-header');


could we use this.$... for all these selectors here? see other usage in this same file

vladikoff · 2015-09-22T01:20:30Z

app/scripts/views/settings.js

@@ -117,7 +117,10 @@ function ($, modal, Cocktail, Session, BaseView, AvatarMixin,
        $.modal.close();
      }
      this.showEphemeralMessages();
+


nit: extra spaces / line breaks, not needed

vladikoff · 2015-09-22T01:22:24Z

Issues with the main header, letters get cut off

vladikoff · 2015-09-22T01:27:55Z

app/scripts/templates/settings.mustache

    {{#showSignOut}}
    <button id="signout" class="settings-button secondary">{{#t}}Sign out{{/t}}</button>
    {{/showSignOut}}
  </header>
+
  <div class="settings-success-wrapper">
    <div class="success settings-success">Hello</div>


Do we still need this Hello here?

it seems like it is not visible

the contents of that get replaced with some "updated" message.

So should this just be <div class="success settings-success"></div> ?

shane-tomlinson · 2015-09-22T10:06:25Z

app/scripts/views/settings.js

@@ -156,6 +159,25 @@ function ($, modal, Cocktail, Session, BaseView, AvatarMixin,
      });
    },

+    _swapDisplayName: function () {


@johngruen - can you document what this does and why it does it?

@shane-tomlinson good idea

johngruen · 2015-09-22T14:38:24Z

@vladikoff so i fixed everything but am unable to repro the text clipping issue in FF or Chrome. Where were did you see this?

vladikoff · 2015-09-23T15:32:43Z

@johngruen Seems like text clipping was fixed but now the headers are just too close to each other:

vladikoff · 2015-09-23T15:35:30Z

@johngruen the above is in latest stable chrome ^

vladikoff · 2015-09-23T15:44:22Z

It seems fine in the "small" view though, a bit close

vladikoff · 2015-09-24T15:25:00Z

Also needs rebase

vladikoff · 2015-09-25T14:35:51Z

app/scripts/views/settings.js

+      $('body.settings').fadeOut(this.FADE_OUT_SETTINGS_MS, function (){
+        $('body').removeClass('settings').show();
+      });
+    },


I think this change will introduce a flash of content, see gif:

@zaach ideas?

Am I going crazy I thought we already had code that did this.

Ugh, I didn't notice in 207dfd6 that we removed the smooth transition.

/cc @shane-tomlinson

This effect is now implemented in base.js, which we could modify to also use a transition if the view specifies a duration.

Ugh, I didn't notice in 207dfd6 that we removed the smooth transition.

/cc @shane-tomlinson

Yeah, that transition messed with the removal of the layoutClassName, so I took it out.

Yeah, that transition messed with the removal of the layoutClassName, so I took it out.

I now see not having it messes with other things. In base.js, can you document why the transition is needed or else another drive by removal could occur.

This flash still exists - can we just remove the fade out for now and file a follow on bug to re-add the behavior more generically?

vladikoff · 2015-09-25T14:37:08Z

app/scripts/views/settings.js

+        cardHeader.text(displayName);
+        cardSubheader.text(email);
+      }
+    },


above is not covered by unit tests, see

https://coveralls.io/builds/3670185/source?filename=app%2Fscripts%2Fviews%2Fsettings.js

Let us know if you need help adding tests for these

shane-tomlinson · 2015-10-01T10:46:20Z

app/scripts/views/settings.js

+        cardHeader.text(email);
+        cardSubheader.text('');
+      } else {
+        cardHeader.text(displayName);


This line and the next are not covered by unit tests:

.

johngruen · 2015-10-01T10:48:40Z

app/tests/spec/views/settings.js

+        });
+      });
+
+      describe('with display name added', function () {


@shane-tomlinson doesn't this cover that?

shane-tomlinson · 2015-10-01T10:51:05Z

app/tests/spec/views/settings.js

+              assert.equal(view.$('.card-subheader').text(), 'a@a.com');
+            });
+        });
+      });


As noted above, one more test is needed that starts with the email address in the header, then adds a display name after the initial render. That should hit the uncovered code.

johngruen · 2015-10-01T11:16:12Z

@shane-tomlinson added another test that should fit the bill, also removed that fading stuff.

shane-tomlinson · 2015-10-01T13:32:35Z

app/scripts/views/settings.js

@@ -148,6 +149,32 @@ function ($, modal, Cocktail, Session, BaseView, AvatarMixin,
      return self._showAvatar();
    },

+    beforeDestroy: function () {


No need for this, it's done automatically over in base.js

fixes #3055

shane-tomlinson · 2015-10-01T14:11:43Z

app/scripts/templates/settings.mustache

@@ -1,24 +1,30 @@
 <div id="fxa-settings-header-wrapper">
  <header id="fxa-settings-header">
-    <h1 id="fxa-manage-account">{{#t}}Manage account{{/t}}</h1>
+    <h1 id="fxa-manage-account">{{#t}}Firefox Account{{/t}}</h1>


Do you want Account or Accounts here. I understand the rationale for Account, but that doesn't seem right.

I prefer "Accounts". It fits in more with the domain and the emails we are sending. You are at a site that has accounts, and maybe one day you'll be able to be signed into multple accounts, like Google.

shane-tomlinson · 2015-10-01T15:22:04Z

As soon as the tests pass, I'm hitting the button.

This is a great update @johngruen!

johngruen · 2015-10-01T15:29:16Z

cool!

vladikoff · 2015-10-01T15:31:27Z

r-

display name evaluates javascript:

shane-tomlinson · 2015-10-01T15:32:13Z

display name evaluates javascript:

oh dear lord. Great catch.

* Change `Firefox Account`=>`Firefox Accounts` * Ensure the displayName is not an XSS vector. * Remove the display name from the settings-unit-summary

fix(styles): make setting header UI better r=vladikoff

johngruen added the waffle:review label Sep 21, 2015

johngruen force-pushed the issue-3055 branch from de50195 to 7b3bce6 Sep 21, 2015

pdehaan reviewed Sep 21, 2015
View changes

rfk added this to the FxA-0: quality milestone Sep 21, 2015

vladikoff self-assigned this Sep 21, 2015

vladikoff reviewed Sep 22, 2015
View changes

vladikoff assigned johngruen and unassigned vladikoff Sep 22, 2015

vladikoff reviewed Sep 22, 2015
View changes

shane-tomlinson reviewed Sep 22, 2015
View changes

johngruen force-pushed the issue-3055 branch 2 times, most recently from 8b4ab83 to c44bd9a Sep 22, 2015

shane-tomlinson assigned vladikoff and unassigned johngruen Sep 23, 2015

vladikoff assigned johngruen and unassigned vladikoff Sep 23, 2015

johngruen force-pushed the issue-3055 branch 2 times, most recently from c618c47 to 0d4e898 Sep 25, 2015

vladikoff reviewed Sep 25, 2015
View changes

johngruen mentioned this pull request Sep 25, 2015

Stretched settings page on short display #3099

Closed

vladikoff reviewed Sep 25, 2015
View changes

johngruen force-pushed the issue-3055 branch from 0d4e898 to dae9575 Sep 30, 2015

johngruen force-pushed the issue-3055 branch from dae9575 to 6e71b6c Oct 1, 2015

shane-tomlinson reviewed Oct 1, 2015
View changes

app/scripts/views/settings.js

cardHeader.text(email);

cardSubheader.text('');

} else {

cardHeader.text(displayName);

This comment has been minimized.

Sign in to view

shane-tomlinson Oct 1, 2015
Member

This line and the next are not covered by unit tests:

.

johngruen reviewed Oct 1, 2015
View changes

app/tests/spec/views/settings.js

});

});

describe('with display name added', function () {

This comment has been minimized.

Sign in to view

johngruen Oct 1, 2015
Author Contributor

@shane-tomlinson doesn't this cover that?

shane-tomlinson reviewed Oct 1, 2015
View changes

johngruen force-pushed the issue-3055 branch from 6e71b6c to f2189b3 Oct 1, 2015

shane-tomlinson reviewed Oct 1, 2015
View changes

fix(styles): make setting header UI better

b907fdc

fixes #3055

shane-tomlinson force-pushed the issue-3055 branch from f2189b3 to b907fdc Oct 1, 2015

shane-tomlinson reviewed Oct 1, 2015
View changes

shane-tomlinson assigned shane-tomlinson and unassigned johngruen Oct 1, 2015

shane-tomlinson force-pushed the issue-3055 branch from c4a4584 to 0d554f3 Oct 1, 2015

feat(client): Follow on /settings updates.

20faf0e

* Change `Firefox Account`=>`Firefox Accounts` * Ensure the displayName is not an XSS vector. * Remove the display name from the settings-unit-summary

shane-tomlinson force-pushed the issue-3055 branch from 0d554f3 to 20faf0e Oct 1, 2015

vladikoff added a commit that referenced this pull request Oct 2, 2015

Merge pull request #3089 from mozilla/issue-3055

ab3147f

fix(styles): make setting header UI better r=vladikoff

vladikoff removed the waffle:review label Oct 2, 2015

vladikoff deleted the issue-3055 branch Oct 2, 2015

mozilla / fxa-content-server Archived

fix(styles): make setting header UI better #3089

fix(styles): make setting header UI better #3089

johngruen commented Sep 21, 2015

johngruen commented Sep 21, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Sep 22, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

johngruen commented Sep 22, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 24, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

johngruen commented Oct 1, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

shane-tomlinson commented Oct 1, 2015

johngruen commented Oct 1, 2015

vladikoff commented Oct 1, 2015

shane-tomlinson commented Oct 1, 2015

mozilla / fxa-content-server Archived

fix(styles): make setting header UI better #3089

fix(styles): make setting header UI better #3089

Conversation

johngruen commented Sep 21, 2015

johngruen commented Sep 21, 2015

This comment has been minimized.

pdehaan Sep 21, 2015 Contributor

This comment has been minimized.

vladikoff Sep 22, 2015 Contributor

This comment has been minimized.

johngruen Sep 22, 2015 Author Contributor

This comment has been minimized.

vladikoff Sep 22, 2015 Contributor

This comment has been minimized.

vladikoff Sep 22, 2015 Contributor

vladikoff commented Sep 22, 2015

This comment has been minimized.

vladikoff Sep 22, 2015 Contributor

This comment has been minimized.

shane-tomlinson Sep 22, 2015 Member

This comment has been minimized.

johngruen Sep 22, 2015 Author Contributor

johngruen commented Sep 22, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 23, 2015

vladikoff commented Sep 24, 2015

This comment has been minimized.

vladikoff Sep 25, 2015 Contributor

This comment has been minimized.

zaach Sep 25, 2015 Contributor

This comment has been minimized.

zaach Sep 25, 2015 Contributor

This comment has been minimized.

zaach Sep 25, 2015 Contributor

This comment has been minimized.

shane-tomlinson Sep 28, 2015 Member

This comment has been minimized.

shane-tomlinson Sep 28, 2015 Member

This comment has been minimized.

shane-tomlinson Oct 1, 2015 Member

This comment has been minimized.

vladikoff Sep 25, 2015 Contributor

This comment has been minimized.

shane-tomlinson Oct 1, 2015 Member

This comment has been minimized.

johngruen Oct 1, 2015 Author Contributor

This comment has been minimized.

shane-tomlinson Oct 1, 2015 Member

johngruen commented Oct 1, 2015

This comment has been minimized.

shane-tomlinson Oct 1, 2015 Member

This comment has been minimized.

shane-tomlinson Oct 1, 2015 Member

This comment has been minimized.

ryanfeeley Oct 1, 2015 Contributor

shane-tomlinson commented Oct 1, 2015

johngruen commented Oct 1, 2015

vladikoff commented Oct 1, 2015

shane-tomlinson commented Oct 1, 2015

pdehaan Sep 21, 2015
Contributor

vladikoff Sep 22, 2015
Contributor

johngruen Sep 22, 2015
Author Contributor

vladikoff Sep 22, 2015
Contributor

vladikoff Sep 22, 2015
Contributor

vladikoff Sep 22, 2015
Contributor

shane-tomlinson Sep 22, 2015
Member

johngruen Sep 22, 2015
Author Contributor

vladikoff Sep 25, 2015
Contributor

zaach Sep 25, 2015
Contributor

zaach Sep 25, 2015
Contributor

zaach Sep 25, 2015
Contributor

shane-tomlinson Sep 28, 2015
Member

shane-tomlinson Sep 28, 2015
Member

shane-tomlinson Oct 1, 2015
Member

vladikoff Sep 25, 2015
Contributor

shane-tomlinson Oct 1, 2015
Member

johngruen Oct 1, 2015
Author Contributor

shane-tomlinson Oct 1, 2015
Member

shane-tomlinson Oct 1, 2015
Member

shane-tomlinson Oct 1, 2015
Member

ryanfeeley Oct 1, 2015
Contributor