Fixes #4724 - Add image editor new feature promotion dialog #4750

punamdahiya · 2018-08-06T23:30:57Z

No description provided.

punamdahiya · 2018-08-06T23:33:28Z

Screenshot showing new edit feature promotion dialog on shot page

punamdahiya · 2018-08-06T23:40:07Z

@ianb PR adds promo dialog that can be used to promote new Screenshots features. Thanks!

flodolo

No issues string-wise

punamdahiya · 2018-08-07T17:54:02Z

Thanks @flodolo updated PR with new copy from Michelle, there is no change in ids

punamdahiya · 2018-08-07T18:23:50Z

@flodolo Updated PR with one last change that adds a new title string as seen in screenshot below. Thanks!

ianb

A couple accessibility things, and other small notes

ianb · 2018-08-08T17:00:33Z

server/src/pages/shot/controller.js

+  }
+  const hasSeen = localStorage.hasSeenPromoDialog;
+  if (!hasSeen && model.enableAnnotations) {
+    localStorage.hasSeenPromoDialog = "1";


I don't think we need to be unduly shy, we could show it several times.

@ianb is there a recommended frequency (2-3 times?)

I will arbitrarily say 3, because 3 is a good number.

or interval like for next 1-2 days from first seen?

ok cool will change it to display for 3 shot page loads

ianb · 2018-08-08T17:06:31Z

server/src/promo-dialog.js

+
+  render() {
+    if (this.props.display) {
+      this.props.promoOpen(this.props.promoType);


Given how promoOpen is implemented, I think this should only happen on componentDidMount – at render time we don't know what's in the document.

componentDidMount have this.props.display as undefined on first render, moved the check inside componentDidUpdate

ianb · 2018-08-08T17:12:02Z

server/src/promo-dialog.js

+      this.props.promoOpen(this.props.promoType);
+      return <div id="promo-dialog-panel" className="promo-panel" >
+        <div className={`${this.props.promoType} promo-box default-color-scheme`}>
+        <a className="box-close" onClick={this.closePanel.bind(this)}></a>


This should probably be a button (though I'm not sure if that matters much), and should have an aria-title or something on it. We don't currently have a string for just "close", but as I think about it, the string should be less ambiguous anyway, like "close notification".

Updated to and use aria-label as "Close Notification"

ianb · 2018-08-08T17:13:48Z

server/src/promo-dialog.js

+        </Localized>
+        <span className="promo-star"></span>
+         <Localized id="promoLink">
+          <span id="promo-link" className="message link" onClick={this.closePanel.bind(this)}>


But this probably should be a link or button, since it is interactable. You could use aria-role, but that's not great, while just making it an <a> or <button> basically solves the accessibility issue.

Updated to and use aria-label as "Try Edit Link"

punamdahiya · 2018-08-09T01:37:50Z

@6a68 Adding you as reviewer on PR updated with @ianb feedback on aria-labels and increasing frequency of notification display to 3 shot page loads

punamdahiya · 2018-08-09T16:10:32Z

server/src/pages/shot/controller.js

+  }
+  let show = false;
+  const count = localStorage.hasSeenPromoDialog;
+  const interval = 1; // in minutes


Now that we have increased frequency of promo display to 3, interval here controls when the next notification should be shown from the last seen , it's set to 1 minute for now

jaredhirsch

Couple high-level things:

The goal of this little promo card is to direct the user's attention to a specific part of the interface we want them to explore. In particular, we want them to try clicking the edit button, so they can get back to the editing tools when the card is gone. So, there shouldn't be a link in the card.

I also think it's best to not show the card again if you either click the edit button, or click the 'x' to dismiss the card. (Just increment the count by 3 in those cases.)

Because the card doesn't have a caret pointing to the edit button, it should be centered under that button at both narrow and wide screen sizes. It currently doesn't seem to be:

Also, I think we'll want to localize all the ARIA labels. Fluent syntax allows for localizations for different attributes on an element, see shotPageShareFacebook for an example for the title attribute.

I'll add a few more comments inline.

jaredhirsch · 2018-08-09T18:45:59Z

locales/en-US/server.ftl

+## Shot Page New Feature Promotion Dialog
+promoTitle = Take Note!
+promoMessage = Updated editing tools let you crop, highlight, and even add text to your shot.
+promoLink = Give it a try


Add a note above each of these strings to ask localizers to choose a short translation, if possible, to better fit into the card.

jaredhirsch · 2018-08-09T18:59:04Z

server/src/pages/shot/controller.js

+
+  if (!count) {
+    localStorage.hasSeenPromoDialog = 1;
+    localStorage.lastPromptTime = now;


I wouldn't bother tracking the time. The goal is to teach through repetition, but having the card randomly come back a minute later just seems confusing. Show it three times, or until the user clicks the edit button or clicks the 'x' to dismiss the card.

jaredhirsch · 2018-08-09T19:00:45Z

server/src/promo-dialog.js

+            Give it a try
+          </a>
+        </Localized>
+        <span className="promo-star"></span>


Instead of two spans and a link, just make this a regular paragraph with emoji at the start and end. There's no need for the fancy ::before CSS stuff.

<p>✨ <Localized id="promoLink">Give it a try</Localized> ✨</p>

jaredhirsch · 2018-08-09T19:03:11Z

static/css/frame.scss

+  }
+
+  a.box-close{
+    float:right;


The parent card is positioned, so just absolutely position this to the top and right, rather than get into the sketch of float-based layout.

jaredhirsch · 2018-08-09T19:05:14Z

server/src/promo-dialog.js

+    if (this.props.display) {
+      return <div id="promo-dialog-panel" className="promo-panel" >
+        <div className={`${this.props.promoType} promo-box default-color-scheme`}>
+        <a className="box-close" aria-label="Close Notification" onClick={this.closePanel.bind(this)}></a>


Indent the children of promo-box

jaredhirsch · 2018-08-09T19:18:41Z

static/css/frame.scss

+    width: 180px;
+    z-index: 999;
+    padding: 10px 5px;
+    text-align:center;


missing space

jaredhirsch · 2018-08-09T19:23:19Z

static/css/frame.scss

+.promo-panel {
+  .edit {
+    right: 50px;
+    top: 75px;


Until we're using this panel in multiple places, it doesn't make sense to abstract out positioning for the edit button. This is confusing as written, since the 'edit' and 'promo-box' classes are applied to the same element. Just get rid of this .edit class and keep all the positioning declarations in one spot.

jaredhirsch · 2018-08-09T19:27:36Z

server/src/promo-dialog.js

+        <div className={`${this.props.promoType} promo-box default-color-scheme`}>
+        <a className="box-close" aria-label="Close Notification" onClick={this.closePanel.bind(this)}></a>
+        <Localized id="promoTitle">
+          <span className="title">Take Note!</span>


Just use a header element here, and override its margins in the CSS. h4 seems like roughly the right size

jaredhirsch · 2018-08-09T19:28:34Z

server/src/pages/shot/view.js

@@ -452,6 +456,24 @@ class Body extends React.Component {
    </reactruntime.BodyTemplate>);
  }

+  promoOpen(promoType) {
+    if (promoType === "edit") {


We don't use the promo card for anything else, so get rid of promoType and associated conditionals.

jaredhirsch · 2018-08-09T19:41:15Z

server/src/pages/shot/view.js

@@ -452,6 +456,24 @@ class Body extends React.Component {
    </reactruntime.BodyTemplate>);
  }

+  promoOpen(promoType) {
+    if (promoType === "edit") {
+      document.querySelector(".button.transparent.edit").style.backgroundColor = "#ededf0";


We definitely want to avoid altering other components directly like this. It makes it really hard to reason about the behavior of each component in isolation. Instead, whenever this promo card is shown, we want to also make sure the highlightEditButton state is set. The parent page is already managing both bits of state in componentDidMount:

componentDidMount() { this.setState({highlightEditButton: this.props.highlightEditButton}); this.setState({promoDialog: this.props.promoDialog}); }

All you need to do is ensure the highlight is set whenever the promo is visible. I think this should work:

componentDidMount() { this.setState({highlightEditButton: this.props.highlightEditButton || this.props.promoDialog}); this.setState({promoDialog: this.props.promoDialog}); }

btw you can just apply the hover style to the edit button when highlightEditButton is true. That'd make the highlight a little more noticeable, even when the card isn't present.

punamdahiya · 2018-08-09T21:47:19Z

@6a68 Updated PR with the feedback, only action item that needs to be thought through is background-color on highlight button same as hover style.

jaredhirsch · 2018-08-09T22:02:37Z

I'd suggest (1) create a 'highlight' class that applies the button hover style (2) use the classnames library to conditionally add that class name if this.state.highlightEditButton is true.

So, instead of this:

// line 363 in pages/shot/view.js
editButton = <Localized id="shotPageEditButton">
  <button className="button transparent edit" title="Edit this image" onClick={ this.onClickEdit.bind(this) } ref={(edit) => { this.editButton = edit; }}></button>
</Localized>;

you'd have something like

const highlightEdit = this.state.highlightEditButton;
editButton = <Localized id="shotPageEditButton">
  <button className={classnames("button", "transparent", "edit", {"highlight": highlightEdit})} title="Edit this image" onClick={ this.onClickEdit.bind(this) } ref={(edit) => { this.editButton = edit; }}></button>
</Localized>;

You'll also need to require the classnames library in the shot view. It's used in the homepage view already, in case you want to poke around at some other examples.

jaredhirsch · 2018-08-09T22:11:47Z

@punamdahiya Looking good! Things to fix:

You'll need to have two different top/right positions for the card, based on the media query that's used to collapse the buttons at narrow screen widths. The card looks centered at narrow width, but wrong at full width:

The "Give it a try" text isn't supposed to be deleted, it just shouldn't be a link. See my comment from earlier.
The promo-dialog component is only used on the shot page, so it should be in the server/src/pages/shot directory, not top-level in the server/src directory.

jaredhirsch · 2018-08-09T22:19:00Z

server/src/pages/shot/view.js

@@ -452,6 +456,11 @@ class Body extends React.Component {
    </reactruntime.BodyTemplate>);
  }

+  promoClose() {
+    this.setState({promoDialog: false});
+    localStorage.hasSeenPromoDialog = "3";


Is there some reason to set this to the string "3" instead of the integer 3?

jaredhirsch · 2018-08-09T22:20:04Z

server/src/pages/shot/view.js

+    // If user clicked edit after seeing new edit tool promo
+    // set counter to max to stop showing notification again
+    if (this.props.promoDialog) {
+      localStorage.hasSeenPromoDialog = "3";


Same issue here, just use the number 3.

jaredhirsch · 2018-08-09T22:22:02Z

static/css/frame.scss

+    margin: 5px 2px;
+  }
+
+  .promo-star:before {


I don't think you need .promo-star any more

jaredhirsch · 2018-08-09T22:22:11Z

static/css/frame.scss

+    right: 2px;
+    cursor: pointer;
+    color: #000;
+    width:15px;


Missing space

punamdahiya · 2018-08-09T23:49:48Z

@6a68 PR has the final set of changes updated. Thanks!

jaredhirsch · 2018-08-10T23:50:09Z

@johngruen Hey, any thoughts on the language here? There's some awkward singular / plural conflict between the two lines. Also, this promo card is effectively part of onboarding, since new users will now always see it the first time they see a shot page; should the wording be tweaked to sound less like upselling new features, and more just advertising the features?

jaredhirsch · 2018-08-11T00:16:28Z

@punamdahiya I opened punamdahiya#2 against your PR to fix a few final things. It can wait until you're back; I think the copy could use another pass before we hand it off to localizers.

clouserw · 2018-08-14T18:42:00Z

* "...text to your shot." -> "...text to your shots."

"Give it a try" -> "Give them a try"

john has more feedback he will write in here

punamdahiya · 2018-08-15T17:17:10Z

@6a68 taking out highlight style in favor of star icon LGTM. Updated PR with final copy and ready for review. Thanks!

jaredhirsch · 2018-08-15T20:40:08Z

server/src/pages/shot/promo-dialog.js

+            Updated editing tools let you crop, highlight, and even add text to your shot.
+          </p>
+        </Localized>
+        <p className="message">✨<Localized id="promoLink"><span className="message-text">Give it a try</span></Localized>✨</p>


Give them a try

thanks, good catch!

jaredhirsch · 2018-08-15T20:40:56Z

server/src/pages/shot/promo-dialog.js

+
+  closePanel(event) {
+    this.props.promoClose();
+    sendEvent("promo-closed");


Whenever a new metrics event is added, it should be documented in https://github.com/mozilla-services/screenshots/blob/master/docs/METRICS.md. Sorry I didn't catch this sooner.

jaredhirsch · 2018-08-15T20:42:05Z

static/css/frame.scss

+    top: 2px;
+    right: 2px;
+    cursor: pointer;
+    color: #000;


use $black instead

jaredhirsch

Two minor nits. Good to land once those are addressed

Changes addressed

…dialog

punamdahiya requested a review from flodolo as a code owner August 6, 2018 23:30

punamdahiya requested a review from ianb August 6, 2018 23:33

flodolo approved these changes Aug 7, 2018

View reviewed changes

punamdahiya force-pushed the 4724_Promotion_Dialog branch from 8ba41ae to 3a91b8b Compare August 7, 2018 17:51

punamdahiya force-pushed the 4724_Promotion_Dialog branch from 3a91b8b to e600ca5 Compare August 7, 2018 18:22

ianb self-assigned this Aug 8, 2018

ianb previously requested changes Aug 8, 2018

View reviewed changes

punamdahiya force-pushed the 4724_Promotion_Dialog branch from e600ca5 to 3709be0 Compare August 8, 2018 23:38

punamdahiya requested a review from jaredhirsch August 9, 2018 01:31

punamdahiya force-pushed the 4724_Promotion_Dialog branch 2 times, most recently from d22f4d0 to f1ee843 Compare August 9, 2018 13:10

jaredhirsch assigned jaredhirsch and unassigned ianb Aug 9, 2018

punamdahiya commented Aug 9, 2018

View reviewed changes

jaredhirsch suggested changes Aug 9, 2018

View reviewed changes

punamdahiya force-pushed the 4724_Promotion_Dialog branch 2 times, most recently from 1104db9 to e564de6 Compare August 9, 2018 21:44

jaredhirsch reviewed Aug 9, 2018

View reviewed changes

static/css/frame.scss Outdated

margin: 5px 2px;

}

.promo-star:before {

Copy link

Member

jaredhirsch Aug 9, 2018

Choose a reason for hiding this comment

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

I don't think you need .promo-star any more

jaredhirsch reviewed Aug 9, 2018

View reviewed changes

static/css/frame.scss Outdated

right: 2px;

cursor: pointer;

color: #000;

width:15px;

Copy link

Member

jaredhirsch Aug 9, 2018

Choose a reason for hiding this comment

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

Missing space

punamdahiya force-pushed the 4724_Promotion_Dialog branch from e564de6 to 6a21453 Compare August 9, 2018 23:49

punamdahiya force-pushed the 4724_Promotion_Dialog branch from 1eb1502 to aa7ca28 Compare August 15, 2018 17:13

jaredhirsch reviewed Aug 15, 2018

View reviewed changes

jaredhirsch approved these changes Aug 15, 2018

View reviewed changes

Fixes mozilla-services#4724 - Add image editor new feature promotion …

431242f

…dialog

punamdahiya force-pushed the 4724_Promotion_Dialog branch from aa7ca28 to 431242f Compare August 15, 2018 21:01

punamdahiya merged commit 07ced7b into mozilla-services:master Aug 15, 2018

Fixes #4724 - Add image editor new feature promotion dialog #4750

Fixes #4724 - Add image editor new feature promotion dialog #4750

Conversation

punamdahiya commented Aug 6, 2018

punamdahiya commented Aug 6, 2018

punamdahiya commented Aug 6, 2018

flodolo left a comment

Choose a reason for hiding this comment

punamdahiya commented Aug 7, 2018

punamdahiya commented Aug 7, 2018

ianb 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

punamdahiya commented Aug 9, 2018

Choose a reason for hiding this comment

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

punamdahiya commented Aug 9, 2018

jaredhirsch commented Aug 9, 2018

jaredhirsch commented Aug 9, 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

punamdahiya commented Aug 9, 2018

jaredhirsch commented Aug 10, 2018

jaredhirsch commented Aug 11, 2018

clouserw commented Aug 14, 2018 • edited by johngruen Loading

punamdahiya commented Aug 15, 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

jaredhirsch left a comment

Choose a reason for hiding this comment

clouserw commented Aug 14, 2018 •

edited by johngruen

Loading