Themes: Remove ThemesHelpers.getSlugFromName #1628
Conversation
It is not possible to reliably determine the theme slug from its name, particularly around the usage of spaces and hyphens. It is much more reliable to use include the theme slug with the data provided to theme selection components.
sirbrillig
added
[Feature Group] Appearance & Themes
| { name: 'Twenty Fifteen', slug: 'twentyfifteen' }, | ||
| { name: 'Sequential', slug: 'sequential' }, | ||
| { name: 'Colinear', slug: 'colinear' }, | ||
| { name: 'Edin', slug: 'edin' }, |
There was a problem hiding this comment.
These are different from the old ones -- any reason for that?
There was a problem hiding this comment.
The ones we were using were actually being passed in as props, but they were static, so I see no reason not to just put them here instead of in steps.js
There was a problem hiding this comment.
Ah right, scrolled over steps.js too quickly :-p
|
Also worth noting that even though I updated it here, |
Yeah, would be cool if the |
|
CC'ing @kwight since I think this affects code written by you :-) |
There's a trial working on that at the moment, so we've postponed it until he's done (at least for now or until DSS is activated permanently). |
|
LGTM |
| propTypes: { | ||
| themeName: React.PropTypes.string.isRequired, | ||
| themeSlug: React.PropTypes.string.isRequired, | ||
| }, |
There was a problem hiding this comment.
Why not just pass theme as the only prop? It'd make it more consistent with the changes in other files, and we could replace the above with
propTypes: {
theme: React.PropTypes.Shape( {
name: React.PropTypes.string.isRequired,
slug: React.PropTypes.string.isRequired
} ).isRequired
}There was a problem hiding this comment.
Ooooh. I didn't know about PropType.Shape. I figured it's good to split them up so it's explicit what is required by this component, but Shape might change that. I'll look into it.
|
This just feels like a lateral move; we remove a function, but now require two pieces of data (and more complex data structure) to get the same result. It just makes me think again that what we really want is an endpoint that can take a name, eg. It all works well though, so I'm good with it for now 👍 |
|
Or, I guess the simpler inverse is using |
kwight
added
[Status] Ready to Merge
and removed
[Status] Needs Review
|
@kwight it is certainly possible to use the API even now to get the theme name, but I think a better approach (since that data could be useful elsewhere and because 1 API request is probably faster than 9) would be to pull a list of all available themes from the API and keep it in a Store as soon as Calypso loads. Then we would have the ability to query that store to get the theme slug for a theme name or vice versa. We'd also be able to validate themes, get screenshot URLs without having to build them, etc. That said, I think adding all that infrastructure is maybe beyond the scope here, and there's already a It seems to me that if all we're trying to do with the API here is get the theme slug for the theme name, and we're entering the theme names manually anyway, why not enter the theme slugs too? However, I don't want to increase complexity needlessly, only if there's a problem that needs fixing, and I think that my thought was: " |
Signup: Themes: Remove ThemesHelpers.getSlugFromName
|
@ockham Thanks; I'm sure those will be taken care of before we're at a point of needing to move on |
It is not possible to reliably determine the theme slug from its name, particularly around the usage of spaces and hyphens (eg:
big-brother,twentythirteen). It is much more reliable to include the theme slug with the data provided to theme selection components.Testing
Make sure the signup flows work: http://calypso.localhost:3000/start, http://calypso.localhost:3000/start/dss
That is, there are no JS errors, the site is correctly created, and the site has the chosen theme.