Update octicon version

[ong6]

What is the purpose of this pull request?

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

Fixes #1639

Overview of changes:

• Update octicon version to version 15.0.1

Anything you'd like to highlight / discuss:

Testing instructions:

Can test this by trying :octicon-file-diff: (which was not working previously)

Proposed commit message: (wrap lines at 72 characters)
Update octicon version

---

Checklist: ☑️

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

[jonahtanjz]

I believe there is no need to increment the version no. We usually only do this during a new release. Same for the rest of the package.json.

[damithc]

@jonahtanjz how do we ensure that the version update happens at the next release? Is it automatic? If not, is it better to update the version number now itself? It might help to detect problems with new versions before the next release.

[jonahtanjz]

Prof @damithc, based on the project management guide for doing a release, the version number will be incremented when running the lerna version --no-push command. From my understanding, I don't think we should be changing the version number when updating dependencies as this will affect a few things, such as test files etc, and also make it harder to track if for example another PR has incremented the number for another dependency update etc.

EDIT: We also need to know the changes that will be added to the next release so that we can increment the version accordingly (either patch, minor or major).

It might help to detect problems with new versions before the next release.

We should still be able to detect issues once the PR is merged to master without changing the version number.

[damithc]

Got it. Thanks for the explanation @jonahtanjz

[ong6]

@jonahtanjz

Hi, I'm not sure why this CI is not working and I can't seem to replicate this on my local machine.

The steps I followed to test this:

1. Run npm run setup
2. Run npm run test

npm run test works fine on my end with no errors

Things I have done to troubleshoot this issue:

1. Run npm run updatetest (does not work)
2. Manually compare the file diff that was given in the error.
   • There was no file differences between them, everything was the same (even line endings)
3. Added log outputs to test if there was any difference in the files. (looks the same to me)

[jonahtanjz]

Hi @ong6,

I was able to replicate the error on my local machine after pulling from your branch.

I ran npm run updatetest and it seems there is an additional rule, overflow: visible;, added to octicons.css in the expected test folders. I've opened a PR to the branch for your reference.

Not sure why this is not reflected on your local machine after running npm run updatetest 🤔

EDIT: You can try running npm run setup from your octicon-update branch and then npm run updatetest to see if the octicons.css has any changes. If that does not work perhaps you can try cloning the markbind repo and set it up again.

[ong6]

Thanks for the help @jonahtanjz it worked after I reset the markbind repo and reset it!

[jonahtanjz]

One minor nit, you can ignore the generated .png files from npm run updatetest. It is an open issue #1130 if you are interested :)

[jonahtanjz]

Hi @ong6, just to check on this, able to ignore the .png files from npm run updatetest for this PR?

[ong6]

@jonahtanjz Oops missed your previous message, will remove them now! Or did you mean just ignoring them in gitignore? I think I will create a new PR if that's the case, seems to be quite hard to revert this.

[jonahtanjz]

Oops missed your previous message, will remove them now! Or did you mean just ignoring them in gitignore? I think I will create a new PR if that's the case, seems to be quite hard to revert this.

Ahh sorry might not have been clear 😅 I meant reverting only the .png files. Something like git checkout <Commit ID before the updatetest> -- packages/cli/test/functional/test_site/**/*.png. Note that if you change the test files of any related diagrams then you do not need to revert the respective generated images. In this case, since the test update did not modify any of the diagrams, then we should ignore the changes of the expected images generated. Hope this clears up 😄

[ong6]

git checkout <Commit ID before the updatetest> -- packages/cli/test/functional/test_site/**/*.png

@jonahtanjz Wow! Didn't think it was so simple to ignore the changes, I guess you learn something new with git every day. Thanks for all the help!

[jonahtanjz]

We will still need to track the changes of the generated diagrams when there are updates to the test .puml files here (add/remove .puml files or simply updating the code in the .puml files etc).

What I meant earlier about "ignoring" the generated images is when there are no changes made to the original .puml test files before running updatetest. In this scenario, we should revert the changes made to the expected images generated after updatetest as the images should not have changed. Sorry for the confusion 😅

Just need to remove this line here and the rest is good 👍

[jonahtanjz]

LGTM 👍