Fixes #4685 - [FxA] Common Header Component #4821

punamdahiya · 2018-08-29T22:10:39Z

No description provided.

punamdahiya · 2018-09-04T22:43:55Z

Updated PR with common header for homepage and My Shots. Looking into complexity of bringing in shots header into common component

chenba · 2018-09-06T16:19:46Z

server/src/header.js

+    if (!this.props.initiatePage) {
+      return (
+        this.props.isFirefox && this.props.hasDeviceId ?
+          <SignInButton isAuthenticated={this.props.hasFxa} initiatePage={this.props.initiatePage} /> : null


To parrot what was mentioned during the synchronous review: initiatePage is not necessary if we always redirect back to the same page. (If we want to keep it as an option to redirect to a different page post-auth, then please rename it to initialPage as "initiate" denotes whether to take some action.)

chenba · 2018-09-06T18:25:41Z

server/src/delete-shot-button.js

+            <Localized id="gDelete">
+              <span>Delete</span>
+            </Localized>
+          </button>


This markup is unnecessarily complicated. A background can be set on the button. Also note that background images are not displayed in high contrast mode. One could argue that the image here is decorative since the button has a text label. But generally speaking, inline images are better for accessibility.

chenba · 2018-09-06T18:53:52Z

server/src/header.js

+    }
+
+    const signin = this.renderFxASignIn();
+    const search = this.props.searchForm && this.props.initiatePage === "shots" ? this.props.searchForm : null;


Just want to +1 on Jared's concern regarding how and where the content of the header is inserted. We're already special casing for My Shots here; it doesn't feel right.

I agree it does seem a bit off to have searchForm moved to Header component when rest of its styles and event handlers are in shotIndex view. Will update to move SearchForm rendering outside of Header component

chenba · 2018-09-06T21:34:15Z

server/src/pages/homepage/view.js

-                  { this.renderGetFirefox() }
-                </div>
-                <div className="banner-image-front" />
+          <Header hasDeviceId={this.props.showMyShots} isFirefox={this.props.isFirefox} hasFxa={this.props.hasFxa} initiatePage="" />


nit: why not just hasDeviceId or showMyShots all the way down?

chenba · 2018-09-06T22:36:55Z

server/src/pages/shot/view.js

@@ -346,13 +346,15 @@ class Body extends React.Component {
    let editButton;
    const highlight = this.state.highlightEditButton ? <div className="edit-highlight" onClick={ this.onClickEdit.bind(this) } onMouseOver={ this.onMouseOverHighlight.bind(this) } onMouseOut={ this.onMouseOutHighlight.bind(this) }></div> : null;

-    const activeFavClass = this.props.expireTime ? "" : "is-fav";
+    const activeFavClass = this.props.expireTime ? "un-fav" : "is-fav";


un-fav only affects the span and is-fav only affects the div. It's confusing when class names are unnecessarily added to elements. Why not have the styles of un-fav be the default and then is-fav to toggle? Then we don't even need to change this line.

chenba · 2018-09-06T23:08:20Z

server/src/pages/shot/view.js

-        </div>
+          { favoriteShotButton }
+          { trashOrFlagButton }
+          { this.props.enableAnnotations ? editButton : null }


Perhaps move this check up to the assignment of editButton.

chenba · 2018-09-06T23:25:27Z

server/src/pages/shot/view.js

+                        <Localized id="gMyShots">
+                          <span>My Shots</span>
+                        </Localized>
+                      </button>;


This can go in an else of the if below.

jaredhirsch · 2018-09-06T23:26:56Z

Just to capture what we discussed in IRC:

Seems like the Header is currently doing a lot. It renders header-related stuff for all of the pages, and also contains page-specific code to decide which parts to render.

One way to simplify things might be to separate a generic Header, that renders any sub-components (passed as props) into a region in the DOM, from page-specific headers that know how to create and render page-specific components into the generic Header. For instance, a ShotPageHeader might render itself by creating components for the shot info and shot page buttons, then passing all those components into the Header, actually returning the Header.

Any components/UI found on multiple pages, like the blue upsell bar, or the login/settings button, could either be part of the generic Header, or be sub-components on their own. UI that only occurs on a single page (like the favorite button) could just be elements created in the specific page's PageHeader, or you could go further and make each control its own component (might be a lot of work for a first pass, though).

I'm not sure this is the best way to break up the complexity, but I think it's one way that could work. There are some nice examples of composing a generic Dialog class with specific Dialog subtypes in the React docs. Happy to chat further, here or on IRC/Slack 👍

punamdahiya · 2018-09-07T06:16:58Z

@chenba @6a68 Thanks for feedback. Updated PR with review comments and below items are in progress

Download button
Shot page main-actions style
Update buttons and localize strings ( ensuring big strings e.g. in German doesn’t break layout)
Review My Shots and Shot page responsive flow
Refactor code to use specific header component ShotPageHeader, MyShotsHeader, HomePageHeader to render generic Header

chenba · 2018-09-07T16:06:02Z

@punamdahiya CI failure is caused by linting errors.

chenba · 2018-09-07T17:25:06Z

server/src/header.js

+  }
+
+  renderFxASignIn() {
+    if (!this.props.initialPage) {


It's unclear this is checking for not the homepage or this.props.initialPage is undefined.

It's checking for both and in either case signIn button on auth redirection will redirect back to homepage which I believe should be ok

With specific headers, this check is no more needed

chenba · 2018-09-10T14:55:24Z

With all the auth related info being passed as props in multiple places, I think this patch is a good candidate for React's Context.

chenba · 2018-09-12T02:40:12Z

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

+      sendEvent("goto-homepage", "navbar", {useBeacon: true});
+      location.href = "/";
+    }
+    // Note: we allow the default action to continue


This comment does not make sense as there is no default action.

chenba · 2018-09-12T14:18:22Z

server/src/pages/shot/view.js

+
+    favoriteShotButton = <Localized id="shotPageFavoriteButton">
+      <button className={`nav-button ${shouldShow} `} onClick={this.onClickFavorite.bind(this)}>
+        <div className={`icon-favorite favorite ${activeFavClass}`} ></div>


Is it not possible to achieve the style we want by wrapping the button in the div, not vice-versa?

chenba · 2018-09-12T14:40:12Z

server/src/pages/shotindex/myshots-header.js

+      </Header>
+    );
+  }
+};


This component can be a stateless component as well.

chenba · 2018-09-12T14:56:09Z

server/src/signin-button.js

@@ -18,30 +18,35 @@ exports.SignInButton = class SignInButton extends React.Component {
  render() {
    if (this.state.displaySettings) {
      return <Localized id="settingsButton">
-        <a className="settings-button" href="/settings" aria-label="Settings">
+        <button className="nav-button icon-settings" tabIndex="0" title="Settings" onClick={this.clickHandler.bind(this)}>


This should remain a link; it's navigation.

Updated to button for its advantages around built-in keyboard accessibility that lets tab between and activate using Return/Enter

Using links navigate the web is fundamental and I don't agree we should do something else. Also, I can tab among links and navigate by pressing Enter. Is that not the case on macOS Firefox?

It works once we provide href, will update to use navigate link. Thanks

chenba · 2018-09-12T14:57:03Z

server/src/signin-button.js

+            <span>Sign In</span>
+          </Localized>
+        </button>
+      </Localized>;


It should be a link here as well.

chenba · 2018-09-12T15:00:57Z

static/css/partials/_header.scss

+
+  display: flex;
+  height: 96px;
+  background-color: #f9f9fa;


$light-default can be used here instead of the hex value.

chenba

Would like some markup and component changes.

punamdahiya · 2018-09-12T17:23:58Z

With all the auth related info being passed as props in multiple places, I think this patch is a good candidate for React's Context.

Creating an auth-context should be useful. auth-context Provider will be filled in respective view files (homepage, myshots, shot) with FxA state to be consumed in SignIn button as shown in this example and marketing blue header .

My understanding benefit to switching to context in this PR is avoid passing auth state as props to signIn button. I can update that as separate commit so that easier to weigh in complexity of this change.

Created #4877 to take this refactoring as separate PR

punamdahiya · 2018-09-13T07:24:25Z

@chenba Updated PR with feedback on markup and using stateless components. Thanks!

chenba · 2018-09-13T18:06:45Z

server/src/header.js

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


Now that #4822 is a thing, this line needs a comma.

chenba · 2018-09-13T18:46:00Z

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

+
+  onClickMyShots() {
+    sendEvent("goto-myshots", "homepage", {useBeacon: true});
+    window.location = "/shots";


Please use a link for navigation.

+1, updated and rebased

chenba · 2018-09-13T18:46:59Z

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

+exports.HomePageHeader.propTypes = {
+  hasFxa: PropTypes.bool,
+  isOwner: PropTypes.bool,
+  isFirefox: PropTypes.bool


Need dangling comma here.

chenba

LGTM. But needs a rebase.

punamdahiya · 2018-09-13T20:32:35Z

@flodolo Can you please review localization changes in the PR. Thanks!

flodolo · 2018-09-13T20:33:48Z

locales/en-US/server.ftl

@@ -11,10 +11,13 @@ gSettings = Settings
 gSignIn = Sign In

 ## Header
-signInButton =
-    .aria-label = Sign In
 settingsButton =


If you change attributes in an existing string, you need to change the ID of the string.

flodolo · 2018-09-13T20:34:40Z

locales/en-US/server.ftl

@@ -123,9 +126,17 @@ shotPageDownloadShot =
    .title = Download
 shotPageEditButton =
    .title = Edit this image
-shotPagefavoriteButton =
+shotPageFavoriteButton =


This would require localizers to retranslate the string. If it's just for cosmetic reasons, I would avoid changing the ID.

flodolo · 2018-09-13T20:35:15Z

locales/en-US/server.ftl

 shotPageDownload = Download
+shotPageDraw = Draw


Is this a verb or a noun? I assume a verb (action), but please add a comment.

flodolo · 2018-09-13T20:35:50Z

locales/en-US/server.ftl

 shotPageDownload = Download
+shotPageDraw = Draw
+shotPageFavorite = Favorite


Same question: verb or noun?

It's a verb, added comment to highlight that

punamdahiya · 2018-09-13T20:57:51Z

@flodolo Thanks for review, updated PR with suggested changes

flodolo

It would be really helpful to keep sequential commits when doing fixes, and only squash (if needed) before merge. Without that, there's no easy way to tell what changed between the two commits.

flodolo · 2018-09-13T21:11:22Z

locales/en-US/server.ftl

 shotPageDownload = Download
+# Note: Draw, Favorite and Delete text below is used on shot page header buttons as a verb (action)


Please use one comment per string (not really needed for 'delete', since there's no space for confusion). Localizers don't see the file, see one string at a time, and the associated comment.

The alternative is to use section comments, but I'd avoid that in this case.

@flodolo Good to know, Done. Thanks!

punamdahiya requested a review from flodolo as a code owner August 29, 2018 22:10

punamdahiya changed the base branch from master to fxa-integration August 29, 2018 22:10

punamdahiya changed the title ~~4685 header~~ [Hold] - Fixes #4685 - [FxA] Common Header Component Aug 29, 2018

punamdahiya force-pushed the 4685-header branch 2 times, most recently from c94ce9c to 6f7e129 Compare September 4, 2018 16:20

punamdahiya changed the base branch from fxa-integration to master September 4, 2018 16:20

punamdahiya force-pushed the 4685-header branch from 6f7e129 to 4a7f276 Compare September 4, 2018 22:37

punamdahiya force-pushed the 4685-header branch from 4a7f276 to 5008428 Compare September 6, 2018 05:56

chenba reviewed Sep 6, 2018

View reviewed changes

punamdahiya force-pushed the 4685-header branch from 5008428 to 8e42064 Compare September 7, 2018 05:55

chenba reviewed Sep 7, 2018

View reviewed changes

punamdahiya force-pushed the 4685-header branch 3 times, most recently from a103eb1 to 92803dc Compare September 7, 2018 20:07

punamdahiya force-pushed the 4685-header branch 5 times, most recently from b8f7d1b to fbcc30d Compare September 10, 2018 23:24

chenba reviewed Sep 12, 2018

View reviewed changes

chenba mentioned this pull request Sep 12, 2018

A/B test textless download button on shot page #3894

Closed

chenba reviewed Sep 12, 2018

View reviewed changes

server/src/pages/shotindex/myshots-header.js Outdated

</Header>

);

}

};

Copy link

Collaborator

chenba Sep 12, 2018

Choose a reason for hiding this comment

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

This component can be a stateless component as well.

chenba reviewed Sep 12, 2018

View reviewed changes

chenba suggested changes Sep 12, 2018

View reviewed changes

punamdahiya force-pushed the 4685-header branch from f0e86f3 to 5f87669 Compare September 12, 2018 16:26

punamdahiya mentioned this pull request Sep 12, 2018

[FxA] Use React Context to capture FxA auth state #4877

Closed

punamdahiya force-pushed the 4685-header branch from 5f87669 to ab5df98 Compare September 13, 2018 07:17

punamdahiya force-pushed the 4685-header branch 2 times, most recently from 3f2df79 to d3ff886 Compare September 13, 2018 07:55

chenba reviewed Sep 13, 2018

View reviewed changes

punamdahiya force-pushed the 4685-header branch from d3ff886 to 545d921 Compare September 13, 2018 20:00

chenba approved these changes Sep 13, 2018

View reviewed changes

punamdahiya force-pushed the 4685-header branch from 545d921 to 62d1fe9 Compare September 13, 2018 20:17

flodolo suggested changes Sep 13, 2018

View reviewed changes

Fixes mozilla-services#4685 - [FxA] Common Header Component

914c800

punamdahiya force-pushed the 4685-header branch from 62d1fe9 to 914c800 Compare September 13, 2018 20:54

flodolo reviewed Sep 13, 2018

View reviewed changes

Localization Feedback

493dfc2

flodolo approved these changes Sep 13, 2018

View reviewed changes

punamdahiya merged commit cc11529 into mozilla-services:master Sep 13, 2018

		shotPageDownload = Download
		# Note: Draw, Favorite and Delete text below is used on shot page header buttons as a verb (action)

Fixes #4685 - [FxA] Common Header Component #4821

Fixes #4685 - [FxA] Common Header Component #4821

Conversation

punamdahiya commented Aug 29, 2018

punamdahiya commented Sep 4, 2018

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

jaredhirsch commented Sep 6, 2018

punamdahiya commented Sep 7, 2018

chenba commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenba commented Sep 10, 2018

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

chenba left a comment

Choose a reason for hiding this comment

punamdahiya commented Sep 12, 2018

punamdahiya commented Sep 13, 2018

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

chenba left a comment

Choose a reason for hiding this comment

punamdahiya commented Sep 13, 2018

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 13, 2018

flodolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment