Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Themes: Add live preview to sheet #5482

Merged
merged 6 commits into from
May 31, 2016
Merged

Themes: Add live preview to sheet #5482

merged 6 commits into from
May 31, 2016

Conversation

seear
Copy link
Contributor

@seear seear commented May 20, 2016 •
edited
Loading

Addresses #3162

To Test

Expected

  • Clicking the LIVE PREVIEW link brings up the preview overlay
  • the button on the preview should perform the appropriate action

screen shot 2016-05-26 at 18 26 25
screen shot 2016-05-26 at 18 26 47
screen shot 2016-05-26 at 18 27 00

@seear seear added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels May 20, 2016
@seear seear added this to the Themes: Showcase M4-ThemeSheets milestone May 20, 2016
@seear seear self-assigned this May 20, 2016
@seear seear mentioned this pull request May 23, 2016
Merged
@seear seear force-pushed the add/theme-sheet-preview branch 2 times, most recently from a3faf2e to 8a1540b Compare May 26, 2016 10:30
@seear seear added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] In Progress labels May 26, 2016
@seear
Copy link
Contributor Author

seear commented May 26, 2016

@folletto: This could do with some expert attention to the styling :)

@folletto
Copy link
Contributor

Ok styled up. :)
Tested in Chrome, Firefox, Safari, Edge, IE11.

I also tried to make an animation when the page loads, but gets reset by the re-rendering apparently. Weird, but it was just a small nice detail. :)

screen shot 2016-05-27 at 20 30 36

@folletto folletto removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label May 27, 2016
ockham
ockham reviewed May 30, 2016
View reviewed changes
client/state/themes/theme-details/reducer.js
@@ -28,6 +28,7 @@ export default ( state = Map(), action ) => {
download: action.themeDownload,
taxonomies: action.themeTaxonomies,
stylesheet: action.themeStylesheet,
demo_uri: action.themeDemoUri,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be demoUri: ..., no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing code uses API format demo_uri:

wp-calypso/client/my-sites/themes/helpers.js

Line 36 in bb13146

return `${theme.demo_uri}?demo=true&iframe=true&theme_preview=true`;

@ockham
Copy link
Contributor

ockham commented May 30, 2016

I tried the entire Preview -> Try & Customize -> Activate flow and ended up with an

Uncaught TypeError: Cannot read property 'id' of undefined

in trackThemeActivation.

@ockham
Copy link
Contributor

ockham commented May 30, 2016

I tried the entire Preview -> Try & Customize -> Activate flow and ended up with an

Uncaught TypeError: Cannot read property 'id' of undefined

in trackThemeActivation.

Hmm, weird, can't repro on second try...

@ockham
Copy link
Contributor

ockham commented May 30, 2016

Noting here that on my MBP's 15" screen, I'm always getting a full screen preview, so my experience doesn't quite match this screenshot:

screen shot 2016-05-26 at 18 27 00

@ockham
Copy link
Contributor

ockham commented May 30, 2016

Looking forward to this!

ockham
ockham reviewed May 30, 2016
View reviewed changes
client/components/web-preview/index.jsx
@@ -1,3 +1,5 @@
/** @ssr-ready **/

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of weird to mark this file as @ssr-ready since it contains references to window and document. OTOH, they're wrapped in lifecycle methods, and this PR demonstrate that SSR works, so I guess it's fine :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind of weird to mark this file as @ssr-ready since it contains references to window and document

Yeah, I can't say I'm totally happy with this situation. Seems like it could be fragile. Let's see how it goes...

@folletto
Copy link
Contributor

Noting here that on my MBP's 15" screen, I'm always getting a full screen preview, so my experience doesn't quite match this screenshot:

For reference: the screenshot doesn't show the default. By default opens full screen, which is the expected behaviour. :)

@seear
Copy link
Contributor Author

seear commented May 31, 2016

I tried the entire Preview -> Try & Customize -> Activate flow and ended up with an

Uncaught TypeError: Cannot read property 'id' of undefined
in trackThemeActivation.

I see these from time to time. I don't think it is related to this PR. Next time I see it I'll track it in a separate issue.

I thought it might be related to activating an already activated theme, but that doesn't give me the error.

@seear
Copy link
Contributor Author

seear commented May 31, 2016

@folletto: Nice work on the styling 👍

@ockham
Copy link
Contributor

ockham commented May 31, 2016

Okay, LGTM!

@ockham ockham added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 31, 2016
@seear seear force-pushed the add/theme-sheet-preview branch from 049df2b to a3a423a Compare May 31, 2016 12:41
@seear seear merged commit a3c4c3f into master May 31, 2016
@seear seear deleted the add/theme-sheet-preview branch May 31, 2016 12:48
@seear seear mentioned this pull request May 31, 2016
Closed
4 tasks
aduth
aduth reviewed Aug 8, 2016
View reviewed changes
client/lib/touch-detect/index.js
@@ -1,3 +1,5 @@
/** @ssr-ready **/
Copy link
Contributor

Choose a reason for hiding this comment

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

The changes here don't necessarily make it SSR-ready, does it? The condition below will still throw ReferenceError on the window reference. I think you want 'undefined' !== typeof window instead?

@okmttdhr okmttdhr mentioned this pull request Aug 2, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@seear seear

Labels
[Feature Group] Appearance & Themes Features related to the appearance of sites.
Projects
None yet
Milestone
Themes: Showcase M4-ThemeSheets
Development

Successfully merging this pull request may close these issues.
5 participants
@seear @folletto @ockham @aduth @matticbot