Themes: Add site-selector modal to theme sheet

[seear]

To Test

• Go to http://calypso.localhost:3000/design, with no site selected
• click on Details for a free theme
• click the Pick this design button

Expected: Site selector modal dialog should appear

• pick a site
• click Activate

Expected: URL will change to selected site, Thanks modal will appear

• Go back to http://calypso.localhost:3000/design, with no site selected
• click on Details for a free theme
• click Pick this design button

Expected: Site selector as before, which will end up at the checkout

[ockham]

Don't they all have that display field? At least the premium ones?

[ockham]

I'd love if we could do without bf4a8c4, since the issue will hit us again for everything that runs i18n.translate in module scope on the server, and I don't think we should have to guard it in functions everywhere. Rather, I hope we can init i18n earlier. If we're lucky, we only need to change middleware order here. I'll see if that works.

[seear]

since the issue will hit us again for everything that runs i18n.translate in module scope on the server

Is that common? Seems to me like this code is wrong in translating on build. If we ever sort out i18n on the server we will want to translate dynamically, no?

[ockham]

Hmm, interesting point. I guess I was inspired by copying client behavior, where IIRC i18n is set up during boot. Need to give that some more thought.

Anyway, this code seems to make the PR work without bf4a8c4: https://gist.github.com/ockham/24688ac1317220c23f5e8fbdc0d9b26a

[seear]

@ockham: Should we go with bf4a8c4 or https://gist.github.com/ockham/24688ac1317220c23f5e8fbdc0d9b26a for the i18n?

I don't have a strong opinion either way.

[ockham]

I think the gist I posted initializes i18n at a level that's more consistent with client behavior, so I'd be inclined to go with that.

[seear]

@ockham:

I've applied that gist to branch add/theme-sheet-selector-temp

Still getting following error:

/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/lib/mixins/i18n/index.js:272
    return i18nState.jed[ jedMethod ].apply( i18nState.jed, jedArgs );
                     ^
TypeError: Cannot read property 'gettext' of undefined
    at getTranslationFromJed (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/lib/mixins/i18n/index.js:272:22)
    at Object.translate (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/lib/mixins/i18n/index.js:356:17)
    at Object.<anonymous> (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/my-sites/themes/action-labels.js:10:15)
    at __webpack_require__ (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/webpack/bootstrap 895f2a1ebdfac463d7a7:19:1)
    at Object.<anonymous> (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/my-sites/theme/main.jsx:35:81)
    at __webpack_require__ (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/webpack/bootstrap 895f2a1ebdfac463d7a7:19:1)
    at Object.<anonymous> (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/my-sites/theme/controller.jsx:7:33)
    at __webpack_require__ (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/webpack/bootstrap 895f2a1ebdfac463d7a7:19:1)
    at Object.<anonymous> (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/client/my-sites/theme/index.node.js:5:40)
    at __webpack_require__ (/Users/seear/scratch/a8c_repos/wp-calypso/build/webpack:/webpack/bootstrap 895f2a1ebdfac463d7a7:19:1)
make: *** [run] Error 1

Am I missing something?

[ockham]

Am I missing something?

No, my bad, since I forgot to unstub i18n in my gist. Since section modules are dynamically required in server/pages, while the i18n.init() call is wrapped in an app.get(), we'd probably need to move the i18n init to an even more global stage (further up in server/pages, or even server/boot), or make sure it's inside the same app.get() route handler.

However, I had another look inside client/lib/mixins/i18n, and I don't like what I see. There's a global singleton state thing that, among other things, holds the current locale. So moving i18n init to a more global stage is a definite no-no, or every user is going to end up getting pages with the locale the first user accessing Calypso after a deploy had. (I think we haven't seen that in wpcalypso yet due to limited usage, but we don't want to try that in production. I think it's possible to try in dev by starting Calypso, switching language, reloading the theme sheet, and inspecting page source.)

Not sure if this can be cleanly mitigated by moving the init to the more localized app.get() route handler, but I'm thinking our best bet for now will be to NMR i18n by something that returns { translate: identity }. This will allow us to unstub stuff like action-labels, and continue to use translate() elsewhere.

Not entirely sure about the i18n mixin and localize yet, tho. Maybe this warrants a GH issue of its own, in which case I guess it's fine to merge this PR -- but without squashing the action-labels commit with the others so we can more easily revert later.

[ockham]

Or maybe I'm overthinking :-D Turns out i18n.initialize() -- without a bootstrap parameter falls back to the default language as specified in config. So probably not that harmful to have that in a global state on the server after all...

[seear]

it's fine to merge this PR -- but without squashing the action-labels commit with the others so we can more easily revert later.

ok, I'll squash into two commits and merge.

I don't think we need to worry about i18n.initialize() too much until we start using anything other than the default locale on the server.

[ockham]

Pushed another commit to the temp branch that fixes i18n init by moving to server/pages module state, which I now think should be fine.