Fixes #4848 - Header Without text labels #4940

punamdahiya · 2018-09-22T00:41:37Z

No description provided.

punamdahiya · 2018-09-22T00:42:22Z

Header at width > 719px without text labels

chenba · 2018-09-24T18:10:08Z

At narrow widths, the buttons are above the shot title, which is not matching #4848 (comment) Am I confused about what this PR supposed to implement?

punamdahiya · 2018-09-24T18:20:02Z

At narrow widths, the buttons are above the shot title, which is not matching #4848 (comment) Am I confused about what this PR supposed to implement?

@chenba Sorry for confusion, PR is WIP for narrow widths thats why 'Hold' in title. I created the PR to show how header is going to look with buttons without text and gather feedback if it's good to go ahead with this change.

punamdahiya · 2018-09-24T18:27:29Z

@chenba Sorry for confusion, PR is WIP for narrow widths thats why 'Hold' in title. I created the PR to show how header is going to look with buttons without text and gather feedback if it's good to go ahead with this change.
Oops, I thought I had the commit with 'Hold' in the title, just updated to reflect that :)

punamdahiya · 2018-09-26T19:04:02Z

server/src/header.js

@@ -21,7 +21,7 @@ exports.Header = function Header(props) {

 exports.Header.propTypes = {
  hasLogo: PropTypes.bool,
-  children: PropTypes.node.isRequired,
+  children: PropTypes.node,


This addresses server warning in logs when Header doesn't have children

punamdahiya · 2018-09-26T19:04:50Z

server/src/pages/shot/shotpage-header.js

@@ -161,7 +157,7 @@ class EditableTitle extends React.Component {
      <input ref={(input) => this.textInput = input}
        className="shot-title-input"
        style={{minWidth: this.state.minWidth}}
-        type="text" defaultValue={this.props.title} autoFocus="true"
+        type="text" defaultValue={this.props.title} autoFocus={true}


Fixes warning in logs due to boolean value passed as string

punamdahiya · 2018-09-26T19:07:52Z

static/css/frame.scss

+    padding-bottom: 8px;
+  }
+  @media (min-width: 1260px) {
+    max-width: 500px;


Added explicit check to increase title width when window size is more than 1260. Without this check header layout for title width 500px, wraps around when window size is 1000px

punamdahiya · 2018-09-26T20:23:24Z

@chenba @6a68 PR is updated with latest UI header spec, please review. Thanks!

chenba · 2018-09-26T22:45:37Z

Now that we no longer label the buttons, they should use inline images. In Windows high contrast mode, background images are not displayed. For example:

chenba

Use inline images instead of background images.

punamdahiya · 2018-09-27T01:23:29Z

static/css/frame.scss

+  @include respond-to("large") {
+    font-size: 32px;
+    line-height: 39px;
+    max-width: 250px;


@youwenliang As per spec, for large widths (~1000px) with 96px x 96px 7 buttons in header, leaves about 250px space for title with 28px padding on the right for edit icon shown on hover.

Title width changes to 500px as window size increase to > 1260px to show longer text title, will that work?

sounds good to me.

punamdahiya · 2018-09-27T01:27:34Z

static/css/frame.scss

-    line-height: 20px;
-    padding-bottom: 4px;
+  @include respond-to("medium") {
+    max-width: 200px;


Since we are wrapping our header, we can change this to 500px, will update

youwenliang · 2018-09-27T01:38:21Z

@punamdahiya
The icons seem to have more space on the bottom as shown below (should be aligned center)

Also I updated the toolbar icons here, use these instead:
https://github.com/mozilla/testpilot-assets/tree/master/Firefox_Screenshots/00_Assets/Updated_Icons
(the current icons have different stroke weight and look inconsistent)

Not sure if the svg's color should be context-fill though since chenba brought up the high contrast mode issue.

punamdahiya · 2018-09-27T18:12:33Z

Not sure if the svg's color should be context-fill though since chenba brought up the high contrast mode issue.
@youwenliang That will be great if it's possible to tweak svg so that it shows up even somewhat visible in high contrast mode.

@chenba Our background-images not showing in high contast mode is an existing issue, the experience is better than what's in the PR as we are showing a border around the buttons, I will update PR to bring border back. Thanks

chenba · 2018-09-27T19:43:09Z

Our background-images not showing in high contast mode is an existing issue

Since we are making changes to the buttons, I think it's a good time to fix it. What's the problem with using inline images?

punamdahiya · 2018-09-27T21:01:29Z

Our background-images not showing in high contast mode is an existing issue

Since we are making changes to the buttons, I think it's a good time to fix it. What's the problem with using inline images?

Not a big fan of listing image file paths in js and would like to keep it in css as much as possible. If there are other options to make it high contrast mode accessible Iike updating svg file I would prefer that

punamdahiya · 2018-09-28T00:43:03Z

@punamdahiya
The icons seem to have more space on the bottom as shown below (should be aligned center)

Also I updated the toolbar icons here, use these instead:
(the current icons have different stroke weight and look inconsistent)

@youwenliang Toolbar icons are updated, please find screenshot of latest shot page

youwenliang · 2018-09-28T03:03:03Z

@punamdahiya
I think we need to do something like this in the css: http://adrianroselli.com/2018/03/svg-filtering-for-windows-high-contrast-mode.html
Will need to find all the icons in our UI and apply the filter to them though.

punamdahiya · 2018-09-28T06:14:15Z

@punamdahiya
I think we need to do something like this in the css: http://adrianroselli.com/2018/03/svg-filtering-for-windows-high-contrast-mode.html
Will need to find all the icons in our UI and apply the filter to them though.

thanks for sharing Mark, sadly these media queries are only supported in IE and edge , for Firefox on WHCM for now fallback seems to to use <img> tag instead of background-image in css

punamdahiya · 2018-09-28T18:33:42Z

Updated PR to use inline images for buttons inside header. @6a68 @ianb Barry is on PTO today, if you have bandwidth it will be great if you can review the PR. Thanks!

jaredhirsch · 2018-09-28T19:28:06Z

Sure, I can take a look.

punamdahiya · 2018-09-28T19:36:12Z

Thanks! will resolve conflicts and update.

punamdahiya · 2018-09-28T19:49:49Z

Thanks! will resolve conflicts and update.

Done

jaredhirsch

Looking good overall 👍, couple minor things, see inline comments. Feel free to self-merge once you've addressed them.

UX feedback (this might belong in a separate bug):

The promo card looks a little bit cramped, when compared to the spacious elements in the rest of the shot page.

I don't understand why we're aggressively truncating the title and hiding expiry info below 1260px:

Slightly above 1260px:

Slightly below 1260px:

The full-width, centered toolbar palette looks sort of empty to me. What if we just leave the buttons right-aligned in the header, but shrink them to this size?

jaredhirsch · 2018-09-28T19:46:57Z

server/src/pages/homepage/homepage-header.js

@@ -17,20 +17,20 @@ exports.HomePageHeader = class HomePageHeader extends React.Component {
  renderFxASignIn() {
      return (
        this.props.isFirefox && this.props.isOwner ?
-          <SignInButton isAuthenticated={this.props.hasFxa} initialPage="" /> : null
+          <SignInButton isAuthenticated={this.props.hasFxa} initialPage=""
+            staticLink={this.props.staticLink}/> : null


I've gotten used to seeing attributes vertically aligned if they have to wrap onto a second line, like:

<SignInButton isAuthenticated={this.props.hasFxa} initialPage="" staticLink={this.props.staticLink}/>

I like how this approach differentiates indentation for child elements (2 spaces) versus attributes within an element. What do you think? If this sounds good to you, I think it'd be good to apply it to this patch.

Looks like the eslint react plugin has a rule for this: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-indent-props.md

makes sense, updated patch to align attribute with the first prop

jaredhirsch · 2018-09-28T19:47:23Z

server/src/pages/homepage/homepage-header.js

-      </Localized>;
+          <a className="nav-button icon-shots" title="My Shots" href="/shots"
+            onClick={ this.onClickMyShots.bind(this) } tabIndex="0">
+            <img src={this.props.staticLink("/static/img/icon-shots.svg")} />


Should these images have alt text? I'm not sure

Not sure either, in share-button we have similar tags where img is embedded inside link and have omitted img alt in favor of title attribute on link. https://github.com/mozilla-services/screenshots/blob/master/server/src/share-buttons.js#L129

Cool, we can leave it for now, and see if it triggers the a11y linter later

We should add alt text to all of our images for a11y.

Filed #4976 to take it up in Q4 with a11y bugs

jaredhirsch · 2018-09-28T19:53:35Z

server/src/delete-shot-button.js

@@ -107,4 +103,5 @@ exports.DeleteShotButton.propTypes = {
  confirmDeleteHandler: PropTypes.func,
  cancelDeleteHandler: PropTypes.func,
  isIcon: PropTypes.bool,
+  staticLink: PropTypes.func,


Maybe staticLink should be defined in the base header, instead of being passed around, since it seems to be used in all the headers

Slightly confused by above comment, by base header you mean inside header.js, am I missing something here?

jaredhirsch · 2018-09-28T20:09:31Z

static/css/frame.scss

+    @include respond-to("large") {
+      display: none;
+    }
+    @media (min-width: 1260px) {


Let's stick with the respond-to($size) pattern, instead of using a media query directly. You can call this "extra-large".

jaredhirsch · 2018-09-28T20:09:51Z

static/css/frame.scss

+    order: 0;
+  }
+  background-color: $white;
+  border-bottom: 1px solid rgba(#000, 0.1);


Use $black, not #000

jaredhirsch · 2018-09-28T20:10:19Z

static/css/frame.scss

+    max-width: 250px;
+    padding-bottom: 8px;
+  }
+  @media (min-width: 1260px) {


Again, respond-to, not @media

jaredhirsch · 2018-09-28T20:17:08Z

server/src/pages/shot/view.js


    if (this.props.isOwner) {
+      const highlight = this.state.highlightEditButton
+        ? <div className="edit-highlight"


Maybe we should replace this svg highlight with a separate highlighted-pen image (fine to do this in a followup bug)

punamdahiya · 2018-09-29T00:33:37Z

@flodolo for localization changes are removal of strings, please review. Thanks

punamdahiya · 2018-09-29T01:32:52Z

Updated promo card to align below header button bottom border.

It’s hard to accommodate seven 96px sizes button for header width around 1000px, thats why have to hide expire-info for header width between 960px and 1260px which are the breakpoints for large and extra-large widths.

flodolo

(removals are always fine)

punamdahiya · 2018-10-01T19:15:30Z

Thanks @flodolo . @chenba I have updated PR with feedback from @6a68, with slight changes to optimize promo card position. It will be great if you can give a quick look before we land in master. Thanks

chenba

LGTM

clouserw mentioned this pull request Sep 24, 2018

[FxA] Do Not Wrap Long Lines for Button Label Text #4937

Closed

punamdahiya changed the title ~~Fixes #4848 - Header Without text labels~~ [Hold] Fixes #4848 - Header Without text labels Sep 24, 2018

punamdahiya force-pushed the 4848_Header_Responsive branch 3 times, most recently from af93a5f to 8db6018 Compare September 26, 2018 01:25

punamdahiya requested a review from flodolo as a code owner September 26, 2018 01:25

punamdahiya force-pushed the 4848_Header_Responsive branch 4 times, most recently from 072ddb7 to cfbd159 Compare September 26, 2018 19:02

punamdahiya commented Sep 26, 2018

View reviewed changes

punamdahiya changed the title ~~[Hold] Fixes #4848 - Header Without text labels~~ Fixes #4848 - Header Without text labels Sep 26, 2018

punamdahiya requested review from jaredhirsch and chenba September 26, 2018 20:22

chenba suggested changes Sep 26, 2018

View reviewed changes

punamdahiya commented Sep 27, 2018

View reviewed changes

punamdahiya force-pushed the 4848_Header_Responsive branch from cfbd159 to af9d1b8 Compare September 28, 2018 06:11

punamdahiya force-pushed the 4848_Header_Responsive branch 5 times, most recently from 0320a45 to 44dc381 Compare September 28, 2018 18:29

punamdahiya force-pushed the 4848_Header_Responsive branch from 44dc381 to 035eec9 Compare September 28, 2018 19:49

jaredhirsch suggested changes Sep 28, 2018

View reviewed changes

punamdahiya force-pushed the 4848_Header_Responsive branch from 035eec9 to 10d0c99 Compare September 29, 2018 01:20

flodolo approved these changes Oct 1, 2018

View reviewed changes

Fixes mozilla-services#4848 - Header Without text labels

b4abe49

punamdahiya force-pushed the 4848_Header_Responsive branch from 10d0c99 to b4abe49 Compare October 1, 2018 19:11

punamdahiya mentioned this pull request Oct 1, 2018

Add alt tag to inline images used on home, shot and myshots #4976

Closed

chenba approved these changes Oct 1, 2018

View reviewed changes

punamdahiya merged commit 4290950 into mozilla-services:master Oct 1, 2018

Fixes #4848 - Header Without text labels #4940

Fixes #4848 - Header Without text labels #4940

Conversation

punamdahiya commented Sep 22, 2018

punamdahiya commented Sep 22, 2018

chenba commented Sep 24, 2018

punamdahiya commented Sep 24, 2018

punamdahiya commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punamdahiya commented Sep 26, 2018

chenba commented Sep 26, 2018

chenba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youwenliang commented Sep 27, 2018

punamdahiya commented Sep 27, 2018 • edited Loading

chenba commented Sep 27, 2018

punamdahiya commented Sep 27, 2018

punamdahiya commented Sep 28, 2018 • edited Loading

youwenliang commented Sep 28, 2018

punamdahiya commented Sep 28, 2018 • edited Loading

punamdahiya commented Sep 28, 2018

jaredhirsch commented Sep 28, 2018

punamdahiya commented Sep 28, 2018

punamdahiya commented Sep 28, 2018

jaredhirsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punamdahiya commented Sep 29, 2018

punamdahiya commented Sep 29, 2018

flodolo left a comment

Choose a reason for hiding this comment

punamdahiya commented Oct 1, 2018

chenba left a comment

Choose a reason for hiding this comment

punamdahiya commented Sep 27, 2018 •

edited

Loading

punamdahiya commented Sep 28, 2018 •

edited

Loading

punamdahiya commented Sep 28, 2018 •

edited

Loading