Skip to content

Support always-on plugins - #714

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
jamos-tay:always-on-plugins
Mar 28, 2019
Merged

Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom
jamos-tay:always-on-plugins

Conversation

@jamos-tay

@jamos-tayjamos-tay commented Feb 20, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

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

• [X] Enhancement to an existing feature

Fixes#702

What is the rationale for this request?

Certain plugins are unlikely to be excluded from every project
Support for plugins that are always on by default for every project

What changes did you make? (Give an overview)

Added support for default plugins:

  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:

  • Convert anchors to default plugins to test it

jamos-tay
jamos-tay commented Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js Outdated
* Adds anchor links to headers
*/
module.exports = {
postRender: (content, pluginContext, frontMatter, page) => {

@jamos-tayjamos-tayFeb 20, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful

I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation

@marvinchinmarvinchinFeb 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅

What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?

@jamos-tayjamos-tayFeb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P

@yamgentyamgentFeb 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?

marvinchin
marvinchin reviewed Feb 20, 2019
View reviewed changes

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a couple of questions 🙂

Comment threadsrc/Site.js Outdated
.filter(plugin => !_.includes(defaultPlugins, plugin))
.forEach(plugin => this.loadPlugin(plugin, false));
defaultPlugins
.filter(plugin => !this.siteConfig.pluginsContext

@marvinchinmarvinchinFeb 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂

Possibly something like this:

_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)

@jamos-tayjamos-tayFeb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

TIL, thanks

Comment threadsrc/plugins/default/anchors.js Outdated
* Adds anchor links to headers
*/
module.exports = {
postRender: (content, pluginContext, frontMatter, page) => {

@marvinchinmarvinchinFeb 20, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅

What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?

@jamos-tay

jamos-tay commented Feb 22, 2019

Copy link
Copy Markdown
ContributorAuthor

Rebased and updated

@yamgent
yamgent self-requested a review February 25, 2019 01:51
@yamgentyamgent added this to the v1.19.2 milestone Feb 25, 2019
yamgent
yamgent requested changes Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md Outdated
Comment threadsrc/plugins/default/anchors.js Outdated
* Adds anchor links to headers
*/
module.exports = {
postRender: (content, pluginContext, frontMatter, page) => {

@yamgentyamgentFeb 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?

@yamgentyamgent removed this from the v1.19.2 milestone Feb 25, 2019
acjh
acjh reviewed Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js Outdated
* Finds plugins in the site's default plugin folder
*/
function findDefaultPlugins() {
const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);

@acjhacjhFeb 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.
For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.

Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?

@jamos-tayjamos-tayMar 1, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)

src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins

I think this might be better than using a whitelist as that has to be hardcoded...

@acjhacjhMar 1, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?

@yamgentyamgentMar 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?

Sounds better 👍.

@jamos-tayjamos-tayMar 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure no problem

@nicholaschuayunzhinicholaschuayunzhi mentioned this pull request Feb 27, 2019
Merged
@jamos-tay

jamos-tay commented Mar 1, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated and rebased

@jamos-tay

jamos-tay commented Mar 2, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated, rebased

yamgent
yamgent reviewed Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md Outdated
Comment threadsrc/Site.js Outdated
@jamos-tay

jamos-tay commented Mar 7, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated

yamgent
yamgent requested changes Mar 8, 2019
View reviewed changes

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:

"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}

@jamos-tay

jamos-tay commented Mar 13, 2019

Copy link
Copy Markdown
ContributorAuthor

Sorry for the delay, rebased and updated

yamgent
yamgent requested changes Mar 14, 2019
View reviewed changes

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:

Comment threadsrc/Site.js Outdated
nicholaschuayunzhi
nicholaschuayunzhi reviewed Mar 14, 2019
View reviewed changes

@nicholaschuayunzhinicholaschuayunzhi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?

Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay mentioned this pull request Mar 18, 2019
Open
@jamos-tay

jamos-tay commented Mar 18, 2019

Copy link
Copy Markdown
ContributorAuthor

Sorry for lateness, updated

yamgent
yamgent requested changes Mar 21, 2019
View reviewed changes

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apologies for the late reply.

Comment threadsrc/Page.js Outdated
@jamos-tay

jamos-tay commented Mar 26, 2019

Copy link
Copy Markdown
ContributorAuthor

Updated

@yamgentyamgent added this to the v1.22.1 milestone Mar 27, 2019
@yamgent

yamgent commented Mar 28, 2019

Copy link
Copy Markdown
Member

Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.

Meanwhile the proposed commit message:

Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent changed the title Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent merged commit a2f88a2 into MarkBind:masterMar 28, 2019
@acjhacjh mentioned this pull request Dec 18, 2019
Merged
@ang-zeyuang-zeyu mentioned this pull request Mar 23, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes
+1 more reviewer
@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
Support always-on plugins by jamos-tay · Pull Request #714 · MarkBind/markbind · GitHub
Skip to content
Support always-on plugins - #714
Merged
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Mar 28, 2019
Merged
Support always-on plugins#714
yamgent merged 3 commits into
MarkBind:masterfrom 
jamos-tay:always-on-plugins
Conversation
@jamos-tay
@jamos-tayjamos-tay
commented
Feb 20, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature
Fixes#702
What is the rationale for this request?
Certain plugins are unlikely to be excluded from every project

Support for plugins that are always on by default for every project
What changes did you make? (Give an overview)
Added support for default plugins:
  • Reside in the src/plugins/default folder
  • Folder is scanned and all plugins are included
  • Overriding allowed: If there's a plugin with the same name in the project plugin folder, Markbind will use that one. A warning will be displayed if the default plugin is being overridden so they don't accidentally override
  • Can be turned off by supplying off to pluginsContext:
{
...
plugins : {
// Do not need to specify plugin name in plugins
},
pluginsContext : {
"anchors" : {
"off": true
}
}
}

Also:
  • Convert anchors to default plugins to test it
jamos-tay
jamos-tay
commented
Feb 20, 2019
View reviewed changes
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@jamos-tayjamos-tayFeb 20, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I needed access to Page.headingIndexingLevel, so I added a 4th parameter, page which is the Page object itself... Other plugins might need access to page config so this could be helpful
I'm thinking we can use this for internal plugins, but don't document it in user docs, since we don't want to expose authors to the implementation
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, I think I'll do the page config method, I think getDefaultPluginsContext might end up growing pretty large if we have a lot of default plugins =P
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
marvinchin
marvinchin
reviewed
Feb 20, 2019
View reviewed changes
@marvinchinmarvinchin
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just a couple of questions 🙂
Comment threadsrc/Site.js
Outdated

 .filter(plugin => !_.includes(defaultPlugins, plugin))
 .forEach(plugin => this.loadPlugin(plugin, false));
 defaultPlugins
 .filter(plugin => !this.siteConfig.pluginsContext
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Just wondering - would using lodash's get method make this more readable? 🙂
Possibly something like this:
_.get(this.siteConfig,["pluginsContext",plugin,"off"],false)
@jamos-tayjamos-tayFeb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

TIL, thanks
Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@marvinchinmarvinchinFeb 20, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I'm just thinking it might be dangerous to pass the entire Page object down to plugins, since that would allow the plugin to modify or invoke the variables and methods of Page which could be dangerous. 😅
What do you think about passing down a copy of the pageConfig object instead? Or maybe having a helper method getDefaultPluginsContext which constructs the context for default plugins and appends it to the other pluginsContext?
@jamos-tay
jamos-tay
commented
Feb 22, 2019

Copy link


Copy Markdown

ContributorAuthor
Rebased and updated
@yamgent
yamgent
self-requested a review
February 25, 2019 01:51
@yamgentyamgent
added this to the v1.19.2 milestone
Feb 25, 2019
yamgent
yamgent
requested changes
Feb 25, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/plugins/default/anchors.js
Outdated

 * Adds anchor links to headers
 */
module.exports = {
 postRender: (content, pluginContext, frontMatter, page) => {
@yamgentyamgentFeb 25, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This part doesn't seem to be addressed?
@yamgentyamgent
removed this from the v1.19.2 milestone
Feb 25, 2019
acjh
acjh
reviewed
Feb 26, 2019
View reviewed changes
Comment threadsrc/Site.js
Outdated

 * Finds plugins in the site's default plugin folder
 */
function findDefaultPlugins() {
 const globPath = path.join(__dirname, BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME);
@acjhacjhFeb 26, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

This assumes all built-in plugins are default plugins (i.e. turned on by default), which is not true.

For example, Algolia search plugin loads unnecessary files in the site if not used, so it should be turned off by default.
Also, this is known at MarkBind dev time. Shall we use a constant as a whitelist?
@jamos-tayjamos-tayMar 1, 2019•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Currently there's a separate folder for plugins that are always on (BUILT_IN_PLUGIN_FOLDER_NAME vs BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME)
src/
plugins/
default/
always-on.js
... other default plugins
must-be-turned-on.js
... other normal plugins

Plugins that should be off can be placed outside the default folder, where they are treated like normal plugins
I think this might be better than using a whitelist as that has to be hardcoded...
@acjhacjhMar 1, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
@yamgentyamgentMar 2, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Ah. BUILT_IN_PLUGIN_DEFAULT_FOLDER_NAME → BUILT_IN_DEFAULT_PLUGIN_FOLDER_NAME?
Sounds better 👍.
@jamos-tayjamos-tayMar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure no problem
@nicholaschuayunzhinicholaschuayunzhi
mentioned this pull request
Feb 27, 2019
 Merged

@jamos-tay
jamos-tay
commented
Mar 1, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated and rebased
@jamos-tay
jamos-tay
commented
Mar 2, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated, rebased
yamgent
yamgent
reviewed
Mar 3, 2019
View reviewed changes
Comment threaddocs/userGuide/usingPlugins.md
Outdated

Comment threadsrc/Site.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 7, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
yamgent
yamgent
requested changes
Mar 8, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

I think you forgot to remove the prefix when obtaining the names of the default plugins, because the example in the documentation no longer works, and I have to do this instead:
"pluginsContext": {
"markbind-plugin-anchors": {
"off": true
}
}
@jamos-tay
jamos-tay
commented
Mar 13, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for the delay, rebased and updated
yamgent
yamgent
requested changes
Mar 14, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@nicholaschuayunzhi has some additional comments for this PR, let me post my minor nit first:
Comment threadsrc/Site.js
Outdated

nicholaschuayunzhi
nicholaschuayunzhi
reviewed
Mar 14, 2019
View reviewed changes
@nicholaschuayunzhinicholaschuayunzhi
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Sorry for my late comments. Perhaps the tests could be done in separate PR?
Comment threadsrc/Page.js
Comment threadsrc/plugins/default/markbind-plugin-anchors.js
@jamos-tayjamos-tay
mentioned this pull request
Mar 18, 2019

Open

@jamos-tay
jamos-tay
commented
Mar 18, 2019

Copy link


Copy Markdown

ContributorAuthor
Sorry for lateness, updated
yamgent
yamgent
requested changes
Mar 21, 2019
View reviewed changes
@yamgentyamgent
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Apologies for the late reply.
Comment threadsrc/Page.js
Outdated

@jamos-tay
jamos-tay
commented
Mar 26, 2019

Copy link


Copy Markdown

ContributorAuthor
Updated
@yamgentyamgent
added this to the v1.22.1 milestone
Mar 27, 2019
@yamgent
yamgent
commented
Mar 28, 2019

Copy link


Copy Markdown

Member
Travis is currently borked, don't really know why, the builds are all getting stuck. I will return to this PR and merge it when I have the next free time.
Meanwhile the proposed commit message:
Support always-on plugins (#714)
When authors want to use plugins for his website, he or she must
modify site.json to enable them manually. There are no plugins that
are enabled by default.
In the future, there may be built-in MarkBind plugins that should be
enabled by default. For example, we want to move the anchor
functionality into a plugin. However, as there are no default plugins,
MarkBind will have them disabled by default, even though anchors are
a common feature in websites and it would be troublesome for authors
to enable this manually.
Let's add always-on plugins, plugins that are always enabled by default
unless the author specify in site.json to disable them. As a
"proof-of-concept", let's also refactor the anchor logic to an
always-on plugin to demonstrate how always-on plugins can be utilized.

@yamgentyamgent
changed the title
Support always on pluginsSupport always-on pluginsMar 28, 2019
@yamgent
yamgent
merged commit a2f88a2
into
MarkBind:masterMar 28, 2019
@acjhacjh
mentioned this pull request
Dec 18, 2019
 Merged

@ang-zeyuang-zeyu
mentioned this pull request
Mar 23, 2020
 Merged

Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@acjhacjhacjh left review comments
@marvinchinmarvinchinmarvinchin left review comments
@yamgentyamgentyamgent approved these changes

+1 more reviewer

@nicholaschuayunzhinicholaschuayunzhinicholaschuayunzhi left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@jamos-tay@yamgent@acjh@marvinchin@nicholaschuayunzhi