Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n 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;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

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

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

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

Use a simpler :prefix-name: syntax for icon fonts - #680

Merged
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax
Feb 18, 2019
Merged

Use a simpler :prefix-name: syntax for icon fonts#680
yamgent merged 3 commits into
MarkBind:masterfrom
Xenonym:colon-iconfont-syntax

Conversation

@Xenonym

@XenonymXenonym commented Feb 8, 2019

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

Resolves#612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

  1. Add the above sample code to a MarkBind page and markbind serve. The icons should render:

Example render of :prefix-name: icon fonts

Comment threaddocs/index.md Outdated
@Chng-Zhi-Xuan
Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

@acjhacjh 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.

Let's officially deprecate the variables syntax for icons.

Comment threadsrc/lib/markbind/src/lib/markdown-it/index.js Outdated

@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.

A search for {{glyphicon still returns files that are using the old syntax.

Comment threadsrc/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

Seems like there are some icons that are not rendered.

In reusingcontents.md
unrendered-icons

@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

@Xenonym

Xenonym commented Feb 11, 2019

Copy link
Copy Markdown
ContributorAuthor

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

  • In Site.js#FOOTER_DEFAULT:
    'This is a dynamic height footer that supports variables {{glyphicon_tags}}'
  • In Site.test.js#test('Site resolves variables referencing other variables'):
    '<span id="level3">{{glyphicon_plus}}</span>'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Comment threaddocs/index.md
@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

@acjh

acjh commented Feb 13, 2019

Copy link
Copy Markdown
Contributor

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- convert uses of {{ prefix_name }} to :prefix-name:
- modify tests that rely on {{ prefix_name }} to use other variables
- new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}
@Xenonym

Copy link
Copy Markdown
ContributorAuthor

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

  • Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
  • Removed {{education_icon}} from test_site.
  • Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
  • Used a <span> instead of <font> tag for the code example in reusingContents.md since the <font> tag is deprecated.

@Chng-Zhi-XuanChng-Zhi-Xuan 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.

Thanks for the changes, looks good 👍

Good point on the <font> element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor <font> elements in a separate PR.

@yamgentyamgent added this to the v1.18.1 milestone Feb 17, 2019
@yamgent
yamgent merged commit dc53e76 into MarkBind:masterFeb 18, 2019
@Xenonym
Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35
@damithc

Copy link
Copy Markdown
Contributor

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

image

@Chng-Zhi-Xuan

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

 * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
* _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
* _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

@damithc

Copy link
Copy Markdown
Contributor

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

Chng-Zhi-Xuan added a commit to Chng-Zhi-Xuan/markbind that referenced this pull request May 21, 2019
The files were used as a template to build the iconsMap
for using variable syntax to insert icons.
Since MarkBind#680, we have shifted to another syntax for inserting
icons. We made the decision to depreciate the old
variable syntax. Thus it renders the current icon .csv files
as obsolete.
Let's remove these files as well when depreciating the
variable syntax for icons.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use a simpler syntax for glyphicons/fontawesome

5 participants

@Xenonym@Chng-Zhi-Xuan@acjh@damithc@yamgent