Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Bootstrap icons #2494

Merged
merged 7 commits into from
Apr 9, 2024
Merged

Conversation

yiwen101
Copy link
Contributor

@yiwen101 yiwen101 commented Apr 3, 2024

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Support bootstrap icons #1977

Anything you'd like to highlight/discuss:
I need to upload the copy of bootstrap-icons to the expected folder to pass test (and in current code, the expected folder also contains the source code of all other icons supported) That is where the +50 000 comes from.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Support Bootstrap icons


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

  • Major (when you make incompatible API changes)
  • Minor (when you add functionality in a backward compatible manner)
  • Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

  • To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

  • what was the old feature that was made obsolete
  • any replacement feature (if any), and
  • how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

@yiwen101 yiwen101 marked this pull request as draft April 3, 2024 19:41
Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.08%. Comparing base (147899a) to head (7c635bd).

❗ Current head 7c635bd differs from pull request most recent head 1e473bd. Consider uploading reports for the commit 1e473bd to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2494      +/-   ##
==========================================
+ Coverage   51.02%   51.08%   +0.06%     
==========================================
  Files         124      124              
  Lines        5372     5367       -5     
  Branches     1159     1155       -4     
==========================================
+ Hits         2741     2742       +1     
+ Misses       2341     2335       -6     
  Partials      290      290              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yiwen101 yiwen101 marked this pull request as ready for review April 3, 2024 19:58
@damithc
Copy link
Contributor

damithc commented Apr 4, 2024

Thanks for attempting this @yiwen101
This will be a good addition to the different icon sets we support.

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

Thanks for the work @yiwen101!
I think this looks good and the bootstrap icons are very cute.

Small nit about documentation: Could you that bootstrap icons to the list here and also add it to the acknowledgement

package.json Outdated
@@ -1,7 +1,9 @@
{
"name": "root",
"private": true,
"workspaces": ["packages/*"],
"workspaces": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary change

@yucheng11122017 yucheng11122017 force-pushed the master branch 2 times, most recently from cb84513 to 69ec838 Compare April 5, 2024 06:21
Copy link
Contributor

@kaixin-hc kaixin-hc left a comment

Choose a reason for hiding this comment

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

lgtm, though see my comment and update the documentation further!

@@ -55,6 +55,12 @@ MarkBind currently supports Version 6 of Font Awesome (Free plan). For detailed
1. You may also append `~class-name` to the end of the octicon name to add `class="class-name"` property to your Octicon (e.g. `:octicon-git-pull-request~icon-large-red:` will generate an Octicon of class *icon-large-red*). You may then add corresponding CSS to `{root}/_markbind/layouts/{layout-name}/styles.css` to customize the style of your Octicon.
1. If your background is dark, you may use `:octiconlight-*:` to render the icon as white.

###### Using Bootstrap icons
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Lines 3-5 : can you also update in this PR? Version of bootstrap relied on + Maybe delete line 5, which seems too repetitive.
  2. I never noticed before how repetitive these instructions are. I wonder if there is a better way to streamline the instructions? Hmm. Maybe a tooltip or something to see some different example icons would be cool.

Copy link
Contributor

@yucheng11122017 yucheng11122017 left a comment

Choose a reason for hiding this comment

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

LGTM except small nit

@@ -1,6 +1,6 @@
## Icons

<small>%%Acknowledgement: Font Awesome icons are provided by [Font Awesome](https://fontawesome.com/) under their [free license](https://fontawesome.com/license), Glyphicons are provided by [Glyphicons](https://glyphicons.com/) via [Bootstrap 3](https://getbootstrap.com/docs/3.3/), [Octicons](https://octicons.github.com) are copyright of GitHub, and Material icons are provided by [Google Fonts](https://fonts.google.com/icons) via [`material-icons` by Ravindra Marella](https://www.npmjs.com/package/material-icons) under the [Apache license 2.0](https://www.apache.org/licenses/LICENSE-2.0.html).%%</small>
<small>%%Acknowledgement: Font Awesome icons are provided by [Font Awesome](https://fontawesome.com/) under their [free license](https://fontawesome.com/license), Glyphicons are provided by [Glyphicons](https://glyphicons.com/) via [Bootstrap 3](https://getbootstrap.com/docs/3.3/). Bootstrap icons are designed by [@mdo](https://github.com/mdo), maintained by the [Bootstrap Team](https://github.com/orgs/twbs/people) and provided under MIT liscense. [Octicons](https://octicons.github.com) are copyright of GitHub, and Material icons are provided by [Google Fonts](https://fonts.google.com/icons) via [`material-icons` by Ravindra Marella](https://www.npmjs.com/package/material-icons) under the [Apache license 2.0](https://www.apache.org/licenses/LICENSE-2.0.html).%%</small>

MarkBind supports using Font Icons provided by Font Awesome, Glyphicons and GitHub's Octicons.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MarkBind supports using Font Icons provided by Font Awesome, Glyphicons and GitHub's Octicons.
MarkBind supports using Font Icons provided by Font Awesome, Glyphicons, GitHub's Octicons and Bootstrap icons.

@yucheng11122017 yucheng11122017 merged commit e0e9239 into MarkBind:master Apr 9, 2024
7 checks passed
Copy link

github-actions bot commented Apr 9, 2024

@yucheng11122017 Each PR must have a SEMVER impact label, please remember to label the PR properly.

@yucheng11122017 yucheng11122017 added the r.Minor Version resolver: increment by 0.1.0 label Apr 9, 2024
@yiwen101 yiwen101 added r.Patch Version resolver: increment by 0.0.1 r.Minor Version resolver: increment by 0.1.0 and removed r.Minor Version resolver: increment by 0.1.0 r.Patch Version resolver: increment by 0.0.1 labels Apr 18, 2024
@damithc
Copy link
Contributor

damithc commented May 3, 2024

Feature in action https://nus-cs2103-ay2324s2.github.io/website/admin/tp-w9.html

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Minor Version resolver: increment by 0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants