Optimize icon assets #2082 #2357

jmestxr · 2023-08-23T06:53:50Z

What is the purpose of this pull request?

Exclude icon asset CSS files (i.e. font awesome, material-icons, glyphicons, octicons) from being loaded if they are not used in the page

Resolves #2082

Overview of changes:

Implemented filterIconAssets(preVueSsrHtml: string, postVueSsrHtml: string) where:

cheerio is used to check if the html of the page (including its layout, e.g. navbar, site-nav) contains elements that use icon CSS classes
If no element using the CSS class from an icon library, then remove the CSS asset file of that library

Anything you'd like to highlight/discuss:

Testing instructions:

cd into packages/cli/test/functional/test_site. Run markbind serve -d.
To test page content with icons, test the pages: testGlyphiconInPage.html, testFontAwesomeInPage.html, testMaterialIconsInPage.html, testOcticonInPage.html.
To test site layout with icons, test the page testIconsInSiteLayout.html.

Proposed commit message: (wrap lines at 72 characters)
Optimize loading of icon asset files

Checklist: ☑️

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

codecov · 2023-08-23T08:07:13Z

Codecov Report

Merging #2357 (85592f8) into master (19ac4c2) will increase coverage by 0.43%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #2357      +/-   ##
==========================================
+ Coverage   45.22%   45.66%   +0.43%     
==========================================
  Files         123      123              
  Lines        5183     5199      +16     
  Branches     1089     1097       +8     
==========================================
+ Hits         2344     2374      +30     
+ Misses       2520     2506      -14     
  Partials      319      319

Files	Coverage Δ
packages/core/src/Page/PageConfig.ts	`4.54% <ø> (ø)`
packages/core/src/Page/index.ts	`19.41% <88.23%> (+10.46%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jmestxr · 2023-08-30T09:10:41Z

Currently there are some issues faced in cheerio parsing. The content in filterIconAssets is not fully processed as html and hence there are some non-html tags. cheerio is unable to parse these tags and hence it can't detect icons used within those tags. E.g. I have glyphicon icons used within tags which cheerio can't detect.

Any ideas on how to do so? 🤔

jmestxr · 2023-08-31T15:04:02Z

Currently there are some issues faced in cheerio parsing. The content in filterIconAssets is not fully processed as html and hence there are some non-html tags. cheerio is unable to parse these tags and hence it can't detect icons used within those tags. E.g. I have glyphicon icons used within tags which cheerio can't detect.

Any ideas on how to do so? 🤔

Update: It's okay, this is resolved

tlylt · 2023-09-01T03:05:34Z

@jmestxr

Any reason why we are creating a new test site for this?
Can we fix the test case error?
Does this work well during development? i.e when I'm writing content for a page, and I delete/include icons, will the page reload properly, and will there be any performance issue?

jmestxr · 2023-09-01T05:25:40Z

@jmestxr

Any reason why we are creating a new test site for this?

Can we fix the test case error?

Does this work well during development? i.e when I'm writing content for a page, and I delete/include icons, will the page reload properly, and will there be any performance issue?

Hi @tlylt,

I was planning to create more test pages under test_site_optimize_icons, and a sub-site with no icons at all. I think I forgot about this. Will be adding these soon.
I just identified the issue. Cos the MarkBind version in master is updated to v5.0.2 and I have not merged this branch with master. Will be solving this.
I have tried some user testing and the pages reload properly and showing the right icon asset files.

tlylt · 2023-09-01T06:45:32Z

I was planning to create more test pages under test_site_optimize_icons, and a sub-site with no icons at all. I think I forgot about this. Will be adding these soon.

Can we add these in the existing site instead of creating a new one?

jmestxr · 2023-09-02T06:34:11Z

I was planning to create more test pages under test_site_optimize_icons, and a sub-site with no icons at all. I think I forgot about this. Will be adding these soon.

Can we add these in the existing site instead of creating a new one?

The existing test_site contains icons in the site-nav and I plan to create a new site that is clean of icons in the layouts, so I can test the page content independently

tlylt · 2023-09-03T02:05:11Z

I was planning to create more test pages under test_site_optimize_icons, and a sub-site with no icons at all. I think I forgot about this. Will be adding these soon.

Can we add these in the existing site instead of creating a new one?

The existing test_site contains icons in the site-nav and I plan to create a new site that is clean of icons in the layouts, so I can test the page content independently

Is testing the page content "independently" absolutely necessary? I assume you can test the conditions that some pages don't have certain icons? If we have to, can we remove the icons in the layouts? does that impact any existing test cases?

Best not to add a new site when there's no need to, for the sake of reducing noise in the test suite.

packages/cli/test/functional/test_site/site.json

tlylt · 2023-09-22T11:30:47Z

Hi @jmestxr, I observe that with this change, pages that don't include certain icon assets will still have the link tag but with empty href attributes, e.g. on https://deploy-preview-2357--markbind-master.netlify.app/

Is this a concern? Can we properly remove them? Or are there issues with removing them entirely?

jmestxr · 2023-09-25T06:39:54Z

Hi @jmestxr, I observe that with this change, pages that don't include certain icon assets will still have the link tag but with empty href attributes, e.g. on https://deploy-preview-2357--markbind-master.netlify.app/

Is this a concern? Can we properly remove them? Or are there issues with removing them entirely?

Hi @tlylt, I have managed to remove the empty tags.

tlylt · 2023-09-25T14:00:29Z

Hi @jmestxr, I have not gone into the code. Found some issues with modal (and possibly with the "include" feature?)

if you add e.g. :mio-cloud-download: into a modal, then opening it will not render the material icon properly, because the css is not included
- strangely Octicons works even without importing the css stylesheet? Need some investigation here
I have not tested, but need your help to check if I use an icon in a .md file, and include it in another .md file, will the icon work fine?
may also need to check "tab", and "panels"

jmestxr · 2023-10-05T06:57:07Z

Hi @jmestxr, I have not gone into the code. Found some issues with modal (and possibly with the "include" feature?)

if you add e.g. :mio-cloud-download: into a modal, then opening it will not render the material icon properly, because the css is not included

strangely Octicons works even without importing the css stylesheet? Need some investigation here

I have not tested, but need your help to check if I use an icon in a .md file, and include it in another .md file, will the icon work fine?

may also need to check "tab", and "panels"

Hi @tlylt , thanks for catching this. I have updated the code to fix this. In addition, I have tested on modals, panels, quiz. For tabs, this PR isn't the issue that icons could not be rendered.

tlylt · 2023-10-06T00:25:15Z

Hi @jmestxr, I am still seeing issues with modal:

go to preview https://deploy-preview-2357--markbind-master.netlify.app/userguide/components/popups#modals
click on 'Slot' to open the modal, see the close button

For tabs, this PR isn't the issue that icons could not be rendered.

Sorry you mean there is a problem with icon display in tabs right now? 👀, is this reported?

jmestxr · 2023-10-09T02:27:40Z

Hi @jmestxr, I am still seeing issues with modal:

go to preview https://deploy-preview-2357--markbind-master.netlify.app/userguide/components/popups#modals

click on 'Slot' to open the modal, see the close button

@tlylt I did not realise that some vue components also use icons provided by font awesome/glyphicons etc.

I have updated filterIconAssets in Page/index.ts which now checks the presence of these icons in both pre vue-rendered HTML and post vue-rendered HTML. Pre-vue HTML does not include the HTML for vue components after rendering, and post-vue does not include HTML of popups like trigger, modals. Hence, need to check both HTML to account for all the icons.

Sorry you mean there is a problem with icon display in tabs right now? 👀, is this reported?

Yep, I have tried this in the master branch and observed that the slot in the tabs seem to only be processed as text only, e.g.

<tabs>
  <tab header="First tab">
  
    An material icon is supposed to appear here ---> :octicon-git-pull-request:

  </tab>
</tabs>

renders as

Removing the empty newlines in the markdown code still also renders the content as text.

tlylt · 2023-10-09T02:59:39Z

I have updated filterIconAssets in Page/index.ts which now checks the presence of these icons in both pre vue-rendered HTML and post vue-rendered HTML. Pre-vue HTML does not include the HTML for vue components after rendering, and post-vue does not include HTML of popups like trigger, modals. Hence, need to check both HTML to account for all the icons.

Got it, any performance impact that you observe/foresee for this update?

Yep, I have tried this in the master branch and observed that the slot in the tabs seem to only be processed as text only, e.g.

Could you help me raise an issue for this?

jmestxr · 2023-10-09T12:08:24Z

Got it, any performance impact that you observe/foresee for this update?

From what I see, there's no significant change in build time, and time taken to load CSS assets, even for larger icon files like all.min.css (font-awesome styles).

tlylt

Hi @jmestxr, thank you for addressing my earlier comments. I think this is mostly ready. I tested locally and seems like it's able to handle edge cases of Modal/Tooltip/include etc.

I left some comments regarding functional test outputs not being accurate according to the description (they still include the other icons), which hopefully will be the last minor issue to tackle.

Lastly, don't forget to update your PR description.

packages/cli/test/functional/test_site/expected/testFontAwesomeInPage.html

packages/cli/test/functional/test_site/testGlyphiconInPage.md

packages/core/src/Page/index.ts

tlylt

LGTM, thank you @jmestxr

jmestxr added 2 commits August 23, 2023 14:34

Remove icon asset files not used in page

3850e63

Update tests

c3dd3e4

jmestxr marked this pull request as draft August 24, 2023 04:04

Create test site

3fd8c91

jmestxr marked this pull request as ready for review August 30, 2023 09:36

jmestxr marked this pull request as draft August 31, 2023 15:04

Update filterIconAssets to be called on processed HTML

327ccc0

jmestxr marked this pull request as ready for review August 31, 2023 15:10

jmestxr added 2 commits September 1, 2023 13:20

Add more test pages

2954575

Merge branch 'master' into optimize-icon-assets

607dde1

jmestxr marked this pull request as draft September 1, 2023 06:12

Add test-site with no icons

8d83f21

jmestxr added 2 commits September 2, 2023 14:48

Update tests

d4c4221

Update tests

793edb8

jmestxr marked this pull request as ready for review September 2, 2023 06:53

tlylt reviewed Sep 3, 2023

View reviewed changes

packages/cli/test/functional/test_site/site.json Outdated Show resolved Hide resolved

tlylt marked this pull request as draft September 3, 2023 02:06

jmestxr added 4 commits September 4, 2023 20:23

Migrate test pages to existing test_site

cc3539c

Update tests

01d6bf4

Revert typo made in past commit

0f2523e

Merge branch 'master' into optimize-icon-assets

0cd5201

jmestxr marked this pull request as ready for review September 5, 2023 02:05

Remove unused import

28c5a1c

jmestxr marked this pull request as ready for review September 13, 2023 06:20

Merge branch 'master' into optimize-icon-assets

5468b8b

Remove empty <link> tags

4d3c29e

Merge branch 'master' into optimize-icon-assets

832fc1b

jmestxr marked this pull request as draft September 30, 2023 04:49

Update place to filter icon assets

cd00c6a

jmestxr added 2 commits October 6, 2023 10:37

Update filterIconAssets to process pre and post-vue html

43026ec

Remove commented-out code

c780f6b

tlylt marked this pull request as ready for review October 13, 2023 11:21

tlylt requested changes Oct 13, 2023

View reviewed changes

jmestxr marked this pull request as draft October 16, 2023 01:40

jmestxr added 3 commits October 16, 2023 09:50

filter asset files for TEST_MODE

e83d5a1

Add more comments for filterIconAssets

f2e7b5e

Amend typo in testGlyphiconInPage.md

fa5a27d

jmestxr marked this pull request as ready for review October 16, 2023 04:33

tlylt self-requested a review October 16, 2023 12:04

Filter icon assets in vueSsrHtml for TEST_MODE

85592f8

tlylt approved these changes Oct 23, 2023

View reviewed changes

tlylt merged commit 5a73c80 into MarkBind:master Oct 23, 2023
8 checks passed

tlylt added this to the v5.1.1 milestone Oct 23, 2023

tlylt added the r.Patch Version resolver: increment by 0.0.1 label Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize icon assets #2082 #2357

Optimize icon assets #2082 #2357

jmestxr commented Aug 23, 2023 •

edited by tlylt

codecov bot commented Aug 23, 2023 •

edited

jmestxr commented Aug 30, 2023 •

edited

jmestxr commented Aug 31, 2023 •

edited

tlylt commented Sep 1, 2023

jmestxr commented Sep 1, 2023

tlylt commented Sep 1, 2023

jmestxr commented Sep 2, 2023

tlylt commented Sep 3, 2023

tlylt commented Sep 22, 2023

jmestxr commented Sep 25, 2023

tlylt commented Sep 25, 2023

jmestxr commented Oct 5, 2023

tlylt commented Oct 6, 2023 •

edited

jmestxr commented Oct 9, 2023 •

edited

tlylt commented Oct 9, 2023

jmestxr commented Oct 9, 2023

tlylt left a comment

tlylt left a comment

Optimize icon assets #2082 #2357

Optimize icon assets #2082 #2357

Conversation

jmestxr commented Aug 23, 2023 • edited by tlylt

codecov bot commented Aug 23, 2023 • edited

Codecov Report

jmestxr commented Aug 30, 2023 • edited

jmestxr commented Aug 31, 2023 • edited

tlylt commented Sep 1, 2023

jmestxr commented Sep 1, 2023

tlylt commented Sep 1, 2023

jmestxr commented Sep 2, 2023

tlylt commented Sep 3, 2023

tlylt commented Sep 22, 2023

jmestxr commented Sep 25, 2023

tlylt commented Sep 25, 2023

jmestxr commented Oct 5, 2023

tlylt commented Oct 6, 2023 • edited

jmestxr commented Oct 9, 2023 • edited

tlylt commented Oct 9, 2023

jmestxr commented Oct 9, 2023

tlylt left a comment

Choose a reason for hiding this comment

tlylt left a comment

Choose a reason for hiding this comment

jmestxr commented Aug 23, 2023 •

edited by tlylt

codecov bot commented Aug 23, 2023 •

edited

jmestxr commented Aug 30, 2023 •

edited

jmestxr commented Aug 31, 2023 •

edited

tlylt commented Oct 6, 2023 •

edited

jmestxr commented Oct 9, 2023 •

edited