Themes: Split main into single-site, multi-site,and logged-out #2743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ockham
added
[Feature Group] Appearance & Themes
ockham
force-pushed
the
update/themes-split-main-jsx
branch
5 times, most recently
from
881378a to
c66409e
Compare
ockham
changed the title
ockham
force-pushed
the
update/themes-split-main-jsx
branch
from
c66409e to
d42fac6
Compare
ockham
added
[Status] Needs Review
ockham
changed the title
ockham
changed the title
ockham
force-pushed
the
update/themes-split-main-jsx
branch
from
9d7a572 to
bd43cc1
Compare
| <Button primary | ||
| onClick={ this.onButtonClick } | ||
| >{ this.props.buttonLabel }</Button> | ||
| </WebPreview> |
There was a problem hiding this comment.
No component left behind: 7c1ef85
ockham
force-pushed
the
update/themes-split-main-jsx
branch
2 times, most recently
from
7a724f4 to
c9b34ed
Compare
|
Looks good! Can't see anything obviously wrong with the code, and tests of LO/SS/MS/analytics seem fine. Going through the code, I noticed that the separation of concerns is a boon to readability. 👍 |
`my-sites/themes/main.jsx` has been overflowing with imports and methods, most of which were used in only one or two out of the single-site, multi-site, and logged-out scenario. Likewise, `theme-options` had to cater for all of those three cases. This commit hence breaks `main` into 3 components corresponding to each of those cases and moves `getButtonOptions()` to each of them, removing conditionals that would only serve the other cases. Because of fewer imports, methods, and branches in each of those files, they should now be more legible. To avoid redundancies, this PR also introduces `<ThemePreview>` for use with each of the three components.
ockham
force-pushed
the
update/themes-split-main-jsx
branch
from
38d8ea3 to
fe1ba46
Compare
ockham
added a commit
that referenced
this pull request
Feb 2, 2016
Themes: Split main into single-site, multi-site,and logged-out
scruffian
removed
the
[Status] Needs Review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
my-sites/themes/main.jsxhas been overflowing with imports and methods, most of which were used in only one or two out of the single-site, multi-site, and logged-out scenario. Likewise,theme-optionshad to cater for all of those three cases.This PR hence breaks
maininto 3 components corresponding to each of those cases and movesgetButtonOptions()to each of them, removing conditionals that would only serve the other cases. Because of fewer imports, methods, and branches in each of those files, I believe they are now more legible. It's however even more important now to factor out re-usable parts that at least two of those components use. Hence, this PR also introduces<ThemePreview>.Also,
logged-out.jsxwill hopefully come in handy for SSR.Fixes #2714
PS:
controller.jscould obviously still need a makeover after this PR, but since we might end up with a slightly different routing anyway, I'm postponing that.themes-selectionto bundlethemes-listand search for SSR. That might be an obsolete level of abstraction with the newlogged-outcomponent (which also has theme preview) that this PR introduces, so in another iteration, we are probably going to removethemes-selection.lib/themes/helpers.jstomy-sites/themes/(Themes: Move themes/helpers from lib/ to my-sites/ #3055)