Add support for light themes from bootswatch by luyangkenneth · Pull Request #745 · MarkBind/markbind

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth · 2019-03-02T19:02:54Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] New feature

Fixes#547, Fixes#663

High-level approach

The goal here is to enable themes in MarkBind! Building on the discussion in #547, the approach I'm going for is to:

Incorporate bootswatch into the project via its npm package, instead of the other options. This is so that we can manage the dependency via npm, and be able to directly copy the css files without needing to download from a CDN.
During the asset-copying stage of site generation, use the custom bootstrap.min.css of the specified bootswatch theme instead of the default bootstrap file.
Allow users to change the theme of their site by specifying the name of the theme in site.json. At this point, this will be a global setting - no further granularity will be supported e.g. in the frontmatter or layout.

Non-goals

Adapt custom MarkBind components to use the overridden bootstrap css (to be done in a separate PR in the future)

Things I would like your input on (edit: these are resolved, thanks everyone!)

I wanted this to get reviewed at its early draft stage so that we have a consensus about what I'll be working on, and how best to go about it. I would appreciate your feedback! Here are some open questions I have:

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?
Do you have any concerns about the approach / next steps / anything that I've mentioned so far?

Example of what applying a diferent theme looks like

luyangkenneth · 2019-03-02T19:08:06Z

Adding everyone as a reviewer to bring this to your attention. Feel free to involve yourself (or not) in the discussion 🙂

acjh · 2019-03-03T02:58:21Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.

Yes. It is an issue with npm 6: npm/npm#20434

You can prevent that from happening by (temporarily) downgrading to npm 5.
I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Related: https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#troubleshooting (#582)

Ideally we want every theme to look polished. But some of the things we're styling (e.g. code blocks, highlighting, colour of the GitHub logo) are fixed, and it might be super tedious to have to come up with custom or light/dark mode versions of everything. So I think there's some ambiguity regarding the scope of this feature, if we want to support all bootswatch themes nicely. Do you have any thoughts on this?

If we want to claim theming support, then we should have light/dark versions of almost everything.
We don't want to (and cannot) support all Bootswatch themes nicely.

From your screenshot:

code blocks — look fine.
highlighting — not shown.
colour of the GitHub logo — cannot be changed: https://github.com/logos

damithc · 2019-03-03T03:04:54Z

Shall we support only light themes at first? The need of the hour is for all MarkBind sites not to look exactly the same. That can be achieved with a range of light themes using different primary colors?

Xenonym · 2019-03-03T03:18:11Z

Is npm install bootswatch (no args) the right way to add it as a dependency? I feel uncomfortable seeing all the newly loosened version numbers in package-lock.json, and it wasn't immediately obvious to me how I can prevent that from happening.
Yes. It is an issue with npm 6: npm/npm#20434

@acjh @yamgent It's not so much a bug as it is a change in how package-lock.json is written in npm@6. The official advice is to:

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

colour of the GitHub logo — cannot be changed: github.com/logos

The download for the GitHub mark does have a light version for use on dark backgrounds, (and the GitHub navbar uses it) so a white on dark background logo should be OK.

acjh · 2019-03-03T03:55:25Z

I didn't say it is a bug. It is an issue as the change is contentious.

tl;dr: make sure your whole team is on npm@6 or later, do a single npm install to upgrade your requires fields, and you should stop seeing weird diffs like these.

I disagree. But @yamgent can decide for MarkBind.

yamgent · 2019-03-03T16:16:21Z

I suggest that developers use npm 5 for now, especially when modifying package-lock.json. @yamgent?

Let's stick with npm 5 for now and not risk potentially breaking something.

luyangkenneth · 2019-03-14T09:39:14Z

Thanks for your inputs everyone! We have resolved the npm issue separately in #767, and I've updated this PR description accordingly.

As discussed, we'll only support light themes for now. I'm ready to have the code reviewed - please take a look 🙂 And then after everything is ok, I will proceed with writing the user docs.

luyangkenneth · 2019-03-14T09:41:49Z

 }
 ],
 "headingIndexingLevel": 6,
+ "bootswatchTheme": "journal",


This is a temporary change, to illustrate what modifying the theme looks like from the user's perspective.

"theme": "bootswatch-journal" is more future proof, in case we add support for other themes?

Nice, yeah that sounds better 👍

@damithc - Updated! How does this look now?

Looks good 👍

luyangkenneth · 2019-03-14T09:45:19Z

+ 'spacelab',
+ 'united',
+ 'yeti',
+];


These correspond to subfolder names within bootswatch's node_modules folder.
Spelling of the themes are double checked with https://bootswatch.com/api/4.json

Xenonym · 2019-03-14T17:41:43Z

- return fs.copyAsync(this.siteAssetsSrcPath, this.siteAssetsDestPath);
+ const maybeOverrideDefaultBootstrapTheme = () => {
+ const { theme } = this.siteConfig;
+ if (!theme || !Object.prototype.hasOwnProperty.call(SUPPORTED_THEMES_PATHS, theme)) {


Could this check be simplified to _.has(SUPPORTED_THEMES_PATHS, theme)?

Good call, that definitely looks more readable. Done 👍

luyangkenneth · 2019-03-19T19:23:19Z

Docs are up! 🎉

Commit: 28a84fc
Page: https://deploy-preview-745--markbind-master.netlify.com/userguide/themes

damithc · 2019-03-20T00:40:43Z

+- [`bootswatch-spacelab`](https://bootswatch.com/spacelab/)
+- [`bootswatch-united`](https://bootswatch.com/united/)
+- [`bootswatch-yeti`](https://bootswatch.com/yeti/)
+


Should we use images from bootswatch here (with proper acknowledgegements)? e.g.,

I was wondering about that too. The images have straightforward URLs and should be easy to add to our docs. I think it might be reasonable to do that with proper acknowledgements, but I'm not fully sure about that.

Themes are very visual things; it's best if we can provide a visual comparison. Also, we should avoid sending our users to external sites as much as possible.
Alternatively, we can duplicate the files in our site (but acknowledge the source). I think they use MIT license which allows duplication.

Makes sense, I'll go ahead with that then!

luyangkenneth · 2019-03-20T11:57:28Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

damithc · 2019-03-20T12:05:02Z

@damithc I've added the images for easier visual comparison between the themes. How does this look now?

Looks good, although the pics blends too well with the page due to white background in both. Perhaps put the whole lot inside a <box> so that the gray background can define the edge of each pic? Just a thought. I'm OK with the current one too.

luyangkenneth · 2019-03-20T12:17:10Z

Nice, yeah I agree it looks better:

yamgent

LGTM. 👍 Propose a merge commit message?

luyangkenneth · 2019-03-21T03:09:45Z

@yamgent How about the PR title? :)

yamgent · 2019-03-21T03:50:45Z

@yamgent How about the PR title? :)

The PR title alone is insufficient in providing the context of the change.

Proposed merge commit message:

Add built-in support for light themes from bootswatch (#745)
To use any custom Bootstrap themes, authors must manually copy over the
theme's .css files to the website's asset folders, and configure the
layout's head.md to use the .css files.
Let's add built-in support for bootswatch light themes, by providing a
"theme" option in site.json, so that authors can just directly specify
the name of the theme in order to use it. For example:
{ "theme": "bootswatch-cerulean"
}

damithc · 2019-03-21T04:06:00Z

Commit message: you can also mention why only light themes are being added.

luyangkenneth · 2019-03-21T06:10:45Z

Whoops I thought you meant just the title of the merge commit. What you proposed sounds good to me!

Re: light themes, maybe we can append this to what you wrote:

Dark themes from Bootswatch are not yet supported because they require
our custom MarkBind components to inherit the Bootstrap styling classes,
to be done in #782.

Chng-Zhi-Xuan

LGTM. Note that #782 should cover not just page-nav and site-nav but also other custom components that currently do not have any bootstrap classes.

E.G. Questions and Modals among many others. They do not allow customisation of how it looks currently (unlike panels or badges). So you have to implement customisation for them if we want everything within MarkBind to support theming.

damithc · 2019-03-25T06:34:12Z

I'm testing out this feature now. Is it possible to get the navbar to change color too?
Like this:

luyangkenneth · 2019-03-25T08:48:50Z

@damithc I've investigated this briefly and I think this involves modifying our Navbar custom component to use the bg-primary class, in place of the bg-dark class it currently uses.

For example, if we apply the Journal theme (https://bootswatch.com/journal/), the generated website will have a black navbar (the second one in the list) as opposed to the pink navbar that we see in the thumbnail.

If we change the bootstrap class, the default navbar color will also change, based on the default bootstrap theme.

Using `bg-dark` (current)

Using `bg-primary`

But we can probably work around this by modifying how we specify our <navbar> in the user guide's header.md.

Would you like us to move in this direction? / What are your thoughts?

damithc · 2019-03-25T10:01:00Z

Would you like us to move in this direction? / What are your thoughts?

It would be good to take advantage of the different nav bar colors as the navbar plays an important role in making the site look 'different'. It depends on how much effort it will take though. May be senior devs have some thoughts?

acjh · 2019-03-25T12:12:34Z

Let's continue this discussion in #386.

luyangkenneth requested review from Chng-Zhi-Xuan, Xenonym, amad-person, damithc, marvinchin, sijie123 and yamgent March 2, 2019 19:03

marvinchin mentioned this pull request Mar 7, 2019
[RFC] Apply Prettier Formatting #758
Closed

yamgent force-pushed the master branch from 7f35131 to 43a048dCompare March 8, 2019 15:01

luyangkenneth force-pushed the support-bootswatch-themes branch 3 times, most recently from 13edb09 to 2beb325Compare March 14, 2019 08:33

luyangkenneth changed the title ~~[RFC] Add support for bootswatch themes~~Add support for bootswatch themesMar 14, 2019

luyangkenneth changed the title ~~Add support for bootswatch themes~~Add support for light themes from bootswatchMar 14, 2019

luyangkenneth force-pushed the support-bootswatch-themes branch 3 times, most recently from 094300f to fa4ddf4Compare March 14, 2019 09:32

luyangkenneth marked this pull request as ready for review March 14, 2019 09:39

luyangkenneth commented Mar 14, 2019
View reviewed changes

luyangkenneth force-pushed the support-bootswatch-themes branch from fa4ddf4 to 94d2bbbCompare March 14, 2019 10:12

Xenonym reviewed Mar 14, 2019
View reviewed changes

luyangkenneth force-pushed the support-bootswatch-themes branch from 94d2bbb to 9475633Compare March 15, 2019 13:07

damithc reviewed Mar 20, 2019
View reviewed changes

Xenonym reviewed Mar 20, 2019
View reviewed changes

Comment threaddocs/userGuide/themes.md

luyangkenneth added 7 commits March 20, 2019 19:46

npm install bootswatch
8b9a0f2

Add list of supported bootswatch light themes
f306d7a

Enable specifying bootswatch theme in site.json
5b19474

Make theme selection more future-proof
b398966

Use lodash/has to simplify key check
dca5f83

Add page to user docs
d9312a0

Add images for bootswatch themes
bda3be3

luyangkenneth force-pushed the support-bootswatch-themes branch from 28a84fc to bda3be3Compare March 20, 2019 11:50

Wrap theme cards in <box>
9f1ecdc

yamgent approved these changes Mar 21, 2019
View reviewed changes

yamgent added this to the v1.21.1 milestone Mar 21, 2019

Chng-Zhi-Xuan approved these changes Mar 21, 2019
View reviewed changes

luyangkenneth mentioned this pull request Mar 21, 2019
Support for dark bootswatch themes #782
Closed

yamgent merged commit c330dc9 into masterMar 23, 2019

luyangkenneth deleted the support-bootswatch-themes branch March 23, 2019 06:21

luyangkenneth mentioned this pull request Mar 26, 2019
Allow custom colors for top nav bar #386
Closed