-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Editor: Update page templates on theme change #5785
Conversation
3da89e0
to
c4fdef9
Compare
@jeremeylduvall : What was the reason for switching this to "In Progress"? Is it ready for a proper review or did you run into issues? |
@aduth The current version does work. Had a few improvements in mind though after chatting with @drewblaisdell . Since we now can monitor the theme from |
Flipping this back to "Needs Review." I was hoping to remove reference to I assume Everything mentioned in the original post (including how it works) still applies. |
@@ -49,6 +51,10 @@ export default React.createClass( { | |||
|
|||
componentWillReceiveProps( nextProps ) { | |||
if ( nextProps.siteId === this.props.siteId ) { | |||
if ( this.props.currentTheme && this.props.currentTheme.stylesheet !== this.activeTheme ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be checking nextProps
I think, to compare the incoming theme against the currently tracked active theme.
c4fdef9
to
3e5eead
Compare
Thanks @aduth. Definitely correct. Updated now. I did leave the fetch as |
Looks good 👍 Took for a spin and newly introduced fetching logic triggered as I'd have expected when returning to the page editor after switching themes. |
- Addresses #5387 - Connects PageTemplatesData component to currentThemes state - Fetches page templates if currentTheme state doesn't match this.activeTheme
3e5eead
to
f6bcab9
Compare
Fixes #5387
Ideally, we could move the page templates store to Redux, but this could act as a temporary fix. I connected the PageTemplatesData component to the currentTheme state.
currentTheme
is then passed in as a prop usinggetCurrentTheme
. IncomponentWillReceiveProps
, we're checking ifthis.props.currentTheme.stylesheet !== this.activeTheme
. Both of these URLs will be in the formatpub/hemingway-rewritten
for free themes orpremium/affinity
for premium themes. If they're equal, nothing happens. If they're not equal, we trigger a fetch and updatethis.activeTheme
accordingly so we don't refetch unnecessarily.Using
this.props.currentTheme.stylesheet
feels a bit weird/non-intuitive, but it's the best comparison I could find since we're usingtheme_slug
elsewhere asthis.activeTheme
. We don't have a correspondingtheme_slug
on in the theme details currently.To Test
GIF
(I reversed the steps in the GIF)