-
Notifications
You must be signed in to change notification settings - Fork 142
Deprecate theme attribute in site config JSON #2235
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
Conversation
yucheng11122017
left a comment
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.
LGTM! Thank you @itsyme
theme attribute in site config JSON|
Thanks for the PR @itsyme! LGTM 👍 Are there no tests that need to be updated for this? |
I was worried about that as well, but I searched through all instances of Though I found something else in |
packages/core/src/Site/index.js
Outdated
| */ | ||
| copyBootstrapTheme(isRebuild) { | ||
| const { theme } = this.siteConfig; | ||
| const theme = this.siteConfig.style.bootstrapTheme; |
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.
Wondering if we should rename the theme to bootstrapTheme?
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.
I think we should rename it. Makes more sense since the function is called copyBootstrapTheme.
raysonkoh
left a comment
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.
LGTM other than the minor naming issue.
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.
LGTM! Thanks for the quick fix @itsyme
Could you update the commit message before merging? Reasoning being that this is a breaking change, so we may want more context in the commit message for future reference.
What is the purpose of this pull request?
Overview of changes:
Resolves #2232. Fully deprecates
themeattribute in site config JSON.themeattribute in site config JSON has been fully deprecated.To migrate:
Use
"style": {"bootstrapTheme": "..."}instead of"theme": "...".For example,
Original (in site.json):
Updated (in site.json):
Anything you'd like to highlight/discuss:
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Deprecate theme attribute in site config JSON
Theme attribute in site config JSON is supported, though not
documented.
This could be confusing for users when trying to search for the
theme attribute in documentation or when referring to older
MarkBind sites which still use the theme attribute.
Let's deprecate the theme attribute in site config JSON fully so that
all MarkBind sites use style.bootstrapTheme, which is documented.
This prevents confusion for users by standardising the use of
style.bootstrapTheme to set the bootstrap theme.
Checklist: ☑️