Add theme approval step if add-on is available #59

lmorchard · 2018-02-09T22:33:50Z

If ?theme param is available on initial page load, assume that this is a
shared theme. Populate the editor UI with that theme and also stash it as a
pendingTheme.

If the add-on is not installed, pendingTheme will do nothing and the
editor will display the incoming shared theme.

If the add-on is installed and responds with a stored current theme,
that theme will replace the one in the editor. Then, a permission
dialog with a preview of the pendingTheme will be presented.

If the user hits "skip" on the dialog, pendingTheme is discarded.

If the user hits "apply", the pending theme is pushed into the add-on
and replaces the theme in the editor.

Fixes #27

lmorchard · 2018-02-09T22:44:43Z

Okay, this one could use some help or at least some sanity checking:

First thing is that the theme preview in the dialog is not quite right. Still wrestling with the CSS for the doll a little, but would totally accept some help.

Second thing is that there's a flash of changing content on page load that seems hard to work around:

There's different behavior depending on whether the add-on's installed or not. But, we can't ever really know that the add-on is not installed - we only know that it is installed after a response to a message. So, not getting a response means either it's not installed or it's just taking awhile.

So, what happens is that the uninstalled state appears first, and then everything switches to the installed state once a message makes it through from the add-on. That means the editor switches between themes just as the dialog pops up. Also the CTA to install the add-on goes away. The switch is pretty fast on my machine, but it is noticeable.

We might be able to paper this over with a loading spinner, but the only thing we can really do there is just arbitrarily wait a second or two before giving up on hearing from the add-on. Folks without the add-on would just wait.

lmorchard · 2018-02-09T22:52:11Z

src/web/index.js

+  log('Received shared theme');
+  // TODO: figure out if this param theme matches theme from add-on and ignore if so
+  urlDecodeTheme(params.theme).then(theme => {
+    // Set the pending theme - only matters if add-on is installed


This is where the complicated part happens...

lmorchard · 2018-02-09T22:54:01Z

src/web/index.js

-  jsonCodec.decompress(params.theme).then(theme => {
-    store.dispatch(actions.theme.setTheme({ theme }));
+  log('Received shared theme');
+  // TODO: figure out if this param theme matches theme from add-on and ignore if so


Oh also TBD: If the ?theme param matches what the current theme already is, we should skip displaying the permission dialog. Haven't implemented that check yet.

lmorchard · 2018-02-09T23:20:57Z

Hmm, and actually it looks like this flashes the shared theme in the browser temporarily too. That seems like a bug in the probably over-zealous Redux middleware. Will poke at that

lmorchard · 2018-02-13T04:01:48Z

Debugged middleware a bit, think I fixed the bug with accidentally applying the shared theme to the browser. Still thinking through the sequence & race conditions on initial load, though

chuckharmston · 2018-02-13T16:21:32Z

Proposal: we review this PR for its own merits, and defer consideration of the sequencing pending UX on #62.

lmorchard · 2018-02-13T16:31:27Z

src/lib/utils.js

@@ -10,3 +10,8 @@ export const colorToCSS = color => {
    ? `hsl(${h}, ${s}%, ${l}%)`
    : `hsla(${h}, ${s}%, ${l}%, ${color.a * 0.01})`;
 };
+
+export const themesEqual = (themeA, themeB) =>


Not using this yet, which reminds me of the other thing I'm trying to figure out: When to decide that the shared theme matches the current theme.

I tried doing it as kind of a validation step - the pendingTheme state would not accept a value that matches the current theme. But that ran into race conditions in add-on messaging. Could just check on an arbitrary delay and auto-dismiss the accept dialog, but then there's a flash of content

This seems like a good usecase for a selector; just have a boolean prop passed to the <App> that checks the pendingTheme and currentTheme for equality. Then it's just a matter of checking that prop, as passed through the component hierarchy.

I also tried this as a selector. Problem was, as soon as you change any colors, suddenly the pending theme no longer matches the current theme and the approval dialog appears. Been trying to figure out at what point to just ignore / delete the incoming pendingTheme so that doesn't happen.

Could maybe set another state flag once the user has made any edits and ignore the pendingTheme from there

lmorchard · 2018-02-13T17:31:31Z

Okay, tweaked things to add a userHasEdited flag to use as part of the decision as to whether the shared theme dialog should be displayed. Seems to help solve the issue of skipping the dialog if the shared theme matches current theme

lmorchard · 2018-02-13T17:38:20Z

One more quick tweak for clarity & to pass linting and I think I'll leave this alone for a bit

lmorchard · 2018-02-13T17:42:53Z

src/lib/utils.js

@@ -4,9 +4,16 @@ export const makeLog = context => (...args) =>
  // eslint-disable-next-line no-console
  DEBUG && console.log(`[ThemesRFun ${context}]`, ...args);

+export const floorHSLA = color => ({


Planning to revisit this in #50 - the color picker and theme schema use different magnitudes for HSLA values, but the JS floating point math causes the values to drift. And thus, themes that should be identical look non-identical to the shouldOfferPendingTheme selector because insignificant digits have changed. Will play with normalizing this better in #50

chuckharmston

Codewise, I don't have any problems here, and it generally works as described. My only concern is that this is a relatively complex workflow that takes some time to grok.

Could you add some documentation about the internal flow, how that flow is related to the state, and why?

Otherwise 👍 from me!

lmorchard · 2018-02-13T19:45:49Z

Could you add some documentation about the internal flow, how that flow is related to the state, and why?

I tried to describe it in the commit message - is that what you're looking for / should that live somewhere else?

lmorchard · 2018-02-13T19:49:27Z

src/web/index.js

@@ -113,9 +125,22 @@ render(

 const params = queryString.parse(window.location.search);


Might drop a long comment here about the startup process and shared theme handling

chuckharmston · 2018-02-13T19:49:45Z

I tried to describe it in the commit message - is that what you're looking for / should that live somewhere else?

Yep! I'd like that type of thing more formally represented in in-repo documentation, with a bit of the "why" attached to the "what".

lmorchard · 2018-02-13T20:03:02Z

src/web/index.js

@@ -111,11 +123,51 @@ render(
  document.getElementById('root')
 );

+/**


Added this comment for now, will file an issue for further doco

If ?theme param is available on initial page load, assume that this is a shared theme. Populate the editor UI with that theme and also stash it as a pendingTheme. If the add-on is not installed, pendingTheme will do nothing and the editor will display the incoming shared theme. If the add-on is installed and responds with a stored current theme, that theme will replace the one in the editor. Then, a permission dialog with a preview of the pendingTheme will be presented. If the user hits "skip" on the dialog, pendingTheme is discarded. If the user hits "apply", the pending theme is pushed into the add-on and replaces the theme in the editor. Fixes mozilla#27

lmorchard · 2018-02-13T20:08:52Z

Also added a commit to bump the add-on version and plan to do another development branch deployment after merge

lmorchard requested review from chuckharmston and johngruen February 9, 2018 22:33

lmorchard force-pushed the 27-incoming-theme-approval branch from 9519082 to b26f5c7 Compare February 9, 2018 22:51

lmorchard commented Feb 9, 2018

View reviewed changes

lmorchard force-pushed the 27-incoming-theme-approval branch from b26f5c7 to f81e153 Compare February 13, 2018 03:59

lmorchard commented Feb 13, 2018

View reviewed changes

lmorchard force-pushed the 27-incoming-theme-approval branch from f81e153 to 4d8d53e Compare February 13, 2018 17:29

lmorchard force-pushed the 27-incoming-theme-approval branch 3 times, most recently from caa83fc to 0e74196 Compare February 13, 2018 17:35

lmorchard commented Feb 13, 2018

View reviewed changes

chuckharmston approved these changes Feb 13, 2018

View reviewed changes

lmorchard commented Feb 13, 2018

View reviewed changes

lmorchard force-pushed the 27-incoming-theme-approval branch from 0e74196 to 714fd76 Compare February 13, 2018 20:02

lmorchard commented Feb 13, 2018

View reviewed changes

lmorchard force-pushed the 27-incoming-theme-approval branch from 714fd76 to 35d97df Compare February 13, 2018 20:05

lmorchard mentioned this pull request Feb 13, 2018

Add doco for app state flows? #64

Closed

Bump add-on version to 0.0.20

58b0aec

lmorchard merged commit 9d9ba2a into mozilla:master Feb 13, 2018

lmorchard mentioned this pull request Mar 6, 2018

UI issues with the theme preview #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add theme approval step if add-on is available #59

Add theme approval step if add-on is available #59

lmorchard commented Feb 9, 2018

lmorchard commented Feb 9, 2018 •

edited

Loading

lmorchard Feb 9, 2018

lmorchard Feb 9, 2018

lmorchard commented Feb 9, 2018

lmorchard commented Feb 13, 2018

chuckharmston commented Feb 13, 2018

lmorchard Feb 13, 2018

chuckharmston Feb 13, 2018

lmorchard Feb 13, 2018 •

edited

Loading

lmorchard commented Feb 13, 2018

lmorchard commented Feb 13, 2018

lmorchard Feb 13, 2018 •

edited

Loading

chuckharmston left a comment

lmorchard commented Feb 13, 2018

lmorchard Feb 13, 2018

chuckharmston commented Feb 13, 2018

lmorchard Feb 13, 2018

lmorchard commented Feb 13, 2018

		@@ -113,9 +125,22 @@ render(

		const params = queryString.parse(window.location.search);

Add theme approval step if add-on is available #59

Add theme approval step if add-on is available #59

Conversation

lmorchard commented Feb 9, 2018

lmorchard commented Feb 9, 2018 • edited Loading

lmorchard Feb 9, 2018

Choose a reason for hiding this comment

lmorchard Feb 9, 2018

Choose a reason for hiding this comment

lmorchard commented Feb 9, 2018

lmorchard commented Feb 13, 2018

chuckharmston commented Feb 13, 2018

lmorchard Feb 13, 2018

Choose a reason for hiding this comment

chuckharmston Feb 13, 2018

Choose a reason for hiding this comment

lmorchard Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

lmorchard commented Feb 13, 2018

lmorchard commented Feb 13, 2018

lmorchard Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

chuckharmston left a comment

Choose a reason for hiding this comment

lmorchard commented Feb 13, 2018

lmorchard Feb 13, 2018

Choose a reason for hiding this comment

chuckharmston commented Feb 13, 2018

lmorchard Feb 13, 2018

Choose a reason for hiding this comment

lmorchard commented Feb 13, 2018

lmorchard commented Feb 9, 2018 •

edited

Loading

lmorchard Feb 13, 2018 •

edited

Loading

lmorchard Feb 13, 2018 •

edited

Loading