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

Refactor fontawesome icon usage. #6282

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

saurabhg772244
Copy link
Collaborator

@saurabhg772244 saurabhg772244 commented Feb 14, 2025

📑 Summary

  • Registered icons are now embedded as SVG otherwise the icon will still be embedded as <i> tag
  • This will save clients using mermaid from having to include fontawesome in their project.
  • Updated styles to set proper fill color and position for icons

See mermaid-js/mermaid-cli#474

📏 Design Decisions

When text contains a FontAwesome icon reference:

  • Check if the icon is registered in the mermaid configuration.
  • If the icon is available in registered icons, replace with stringified SVG
  • If the icon is not found, return <i> tag with icon class

Update to styles.

  • We had a style defined that adds fill to all path elements inside node class. Now we are overriding this style to have fill: currentColor also reverting stroke and stroke-width to default

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added necessary unit/e2e tests.
  • 📓 have added documentation. Make sure MERMAID_RELEASE_VERSION is used for all new features.
  • 🦋 If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Feb 14, 2025

🦋 Changeset detected

Latest commit: 2fb7bc2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
mermaid Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Feb 14, 2025

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 2fb7bc2
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/67b8c2e2cfcf640008246054
😎 Deploy Preview https://deploy-preview-6282--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

pkg-pr-new bot commented Feb 14, 2025

Open in Stackblitz

npm i https://pkg.pr.new/mermaid-js/mermaid@6282
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/mermaid-zenuml@6282
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/layout-elk@6282
npm i https://pkg.pr.new/mermaid-js/mermaid/@mermaid-js/parser@6282

commit: 2fb7bc2

@saurabhg772244
Copy link
Collaborator Author

Note: Some e2e tests are failing as they were using fontawesome icon in them but the test file was not importing fontawesome cdn

Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 0% with 151 lines in your changes missing coverage. Please review.

Project coverage is 4.45%. Comparing base (0518e9c) to head (2fb7bc2).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
packages/mermaid/src/rendering-util/createText.ts 0.00% 32 Missing ⚠️
packages/mermaid/src/dagre-wrapper/nodes.js 0.00% 15 Missing ⚠️
packages/mermaid/src/diagrams/block/styles.ts 0.00% 14 Missing ⚠️
packages/mermaid/src/diagrams/class/styles.js 0.00% 14 Missing ⚠️
packages/mermaid/src/diagrams/flowchart/styles.ts 0.00% 14 Missing ⚠️
packages/mermaid/src/diagrams/kanban/styles.ts 0.00% 14 Missing ⚠️
...ckages/mermaid/src/diagrams/user-journey/styles.js 0.00% 14 Missing ⚠️
packages/mermaid/src/dagre-wrapper/clusters.js 0.00% 10 Missing ⚠️
packages/mermaid/src/dagre-wrapper/edges.js 0.00% 6 Missing ⚠️
packages/mermaid/src/dagre-wrapper/shapes/util.js 0.00% 6 Missing ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #6282      +/-   ##
==========================================
- Coverage     4.46%   4.45%   -0.02%     
==========================================
  Files          384     385       +1     
  Lines        54262   54384     +122     
  Branches       623     598      -25     
==========================================
- Hits          2425    2423       -2     
- Misses       51837   51961     +124     
Flag Coverage Δ
unit 4.45% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/mermaid/src/dagre-wrapper/createLabel.js 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/index.js 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/edges.js 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/shapes/util.js 0.00% <0.00%> (ø)
packages/mermaid/src/rendering-util/icons.ts 0.00% <0.00%> (ø)
packages/mermaid/src/dagre-wrapper/clusters.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/block/styles.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/class/styles.js 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/flowchart/styles.ts 0.00% <0.00%> (ø)
packages/mermaid/src/diagrams/kanban/styles.ts 0.00% <0.00%> (ø)
... and 3 more

... and 2 files with indirect coverage changes

@sidharthv96
Copy link
Member

image

This increases the size of our bundle significantly!

Copy link

argos-ci bot commented Feb 19, 2025

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 14 changed, 483 added, 473 removed, 2 failures Feb 21, 2025, 6:26 PM

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Awesome work! This will make me very happy in the mermaid-cli project too!

As far as I can tell, your code and approach looks great to me! But I think we need some work before we can merge this.

I think we need to do the following before we merge this PR:

  • E2E tests! Can you write some visual regression tests that work before and after this feature so we can see if there are any visual anomalies? You might want to try various themes too, to make sure the icon colors are still the same as they are currently.
  • Documentation updates:
    • I think you forgot to update your PR description and changeset for a81c318
    • You're not updating mindmaps in this PR. I think this is fine, as long as it's in the PR description, changeset, and docs.
    • Please update the documentation for FontAwesome in https://mermaid.js.org too

Comment on lines 187 to 224
const iconRegex = /(fas|fab|far|fa|fal|fak|fad):fa-([a-z-]+)/g;
const classNameMap = {
fas: 'fa-solid',
fab: 'fa-brands',
far: 'fa-regular',
fa: 'fa',
fal: 'fa-light',
fad: 'fa-duotone',
fak: 'fak',
} as const;
const matches = [...text.matchAll(iconRegex)];
if (matches.length === 0) {
return text;
}

let newText = text;

for (const match of matches) {
const [fullMatch, prefix, iconName] = match;
const className = classNameMap[prefix];
const registeredIconName = `${prefix}:${iconName}`;

try {
const isFreeIcon = await isIconAvailable(registeredIconName);
if (!isFreeIcon) {
log.warn(`Icon ${registeredIconName} is a pro icon.`);
newText = newText.replace(fullMatch, `<i class='${className} fa-${iconName}'></i>`);
continue;
}
const faIcon = await getIconSVG(registeredIconName, undefined, { class: 'label-icon' });
if (faIcon) {
newText = newText.replace(fullMatch, faIcon);
}
} catch (error) {
log.error(`Error processing ${registeredIconName}:`, error);
}
}
return newText;
Copy link
Member

Choose a reason for hiding this comment

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

issue(blocking): I'm not sure why we still need the free/pro difference, or the different class names. Can we change this function to output the exact same thing if users haven't set an icon pack?

suggestion: Maybe something like the following (we should probably also update the function TSDoc comment too):

Suggested change
const iconRegex = /(fas|fab|far|fa|fal|fak|fad):fa-([a-z-]+)/g;
const classNameMap = {
fas: 'fa-solid',
fab: 'fa-brands',
far: 'fa-regular',
fa: 'fa',
fal: 'fa-light',
fad: 'fa-duotone',
fak: 'fak',
} as const;
const matches = [...text.matchAll(iconRegex)];
if (matches.length === 0) {
return text;
}
let newText = text;
for (const match of matches) {
const [fullMatch, prefix, iconName] = match;
const className = classNameMap[prefix];
const registeredIconName = `${prefix}:${iconName}`;
try {
const isFreeIcon = await isIconAvailable(registeredIconName);
if (!isFreeIcon) {
log.warn(`Icon ${registeredIconName} is a pro icon.`);
newText = newText.replace(fullMatch, `<i class='${className} fa-${iconName}'></i>`);
continue;
}
const faIcon = await getIconSVG(registeredIconName, undefined, { class: 'label-icon' });
if (faIcon) {
newText = newText.replace(fullMatch, faIcon);
}
} catch (error) {
log.error(`Error processing ${registeredIconName}:`, error);
}
}
return newText;
// The letters 'bklrs' stand for possible endings of the fontawesome prefix (e.g. 'fab' for brands, 'fak' for fa-kit) // cspell: disable-line
return text.replace(
/(fa[bklrs]?):fa-([\w-]+)/g, // cspell: disable-line
(fullMatch, prefix, iconName) => {
const registeredIconName = `${prefix}:${iconName}`;
if (await isIconAvailable(registeredIconName)) {
return await getIconSVG(registeredIconName, undefined, { class: 'label-icon' });
} else {
// fall back to FontAwesome CSS
return `<i class='${s.replace(':', ' ')}'></i>`;
}
},
);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will update regex but as of the text.replace() does not take async call back so we will have to update with other way.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense! Good catch.

The next question is, can we write it in such a way that uses Promise.all() to download these icon packets in parallel? (although maybe it's overkill to write the code like that, since it will only result in a performance improvement if you're using multiple icon packs in the same node label).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 2fb7bc2

const expected =
"This is an icon: <i class='fa fa-user'></i> and <i class='fab fa-github'></i>";
const output = await replaceIconSubstring(input);
const expected = `This is an icon: <i class='fa fa-user'></i> and <i class='fa-brands fa-github'></i>`;
Copy link
Member

Choose a reason for hiding this comment

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

question(blocking): Why has the class changed from fab to fa-brands and is this a breaking change? Do we need to put this info in the PR description and changeset entry?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 16573d9

pnpm-lock.yaml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

suggestion(non-blocking but recommended): I think you can revert these changes, since it looks like these pnpm-lock.yaml are not needed for your PR, but will just increase the risk of merge conflicts by a lot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated in 83a6e69

Sign up for free to 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.

3 participants