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

Move buggy bootstrap-icons dependency to core/package.json #2539

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

kaixin-hc
Copy link
Contributor

@kaixin-hc kaixin-hc commented Apr 30, 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:

Addresses #2538

Overview of changes:
Moving of the dependency. The file packages/core/package.json is also where the other icon dependencies live.

Anything you'd like to highlight/discuss:
See comments on issue. Also, testing may be misleading CI passing, local passing even with the erroneous code, and some websites deploying fine on 5.5.0 (eg markbind website).

Also, I have no idea why that last bracket is marked as a change. It looks the same to me???

Testing instructions:
Ideas on how else to test appreciated.

Reproducing issue notes:

  1. You can verify this issue exists locally by making sure you are using the npm installed version and that you have no other markbind on your device + markbind is not npm linked to your local development repository
  2. When I linked to markbind 5.5.0 locally, this crashes... until I ran npm setup, which made all sites work

Testing this PR:

  1. Locally, npm run setup after these changes results in the local sites working 😓 😅

Proposed commit message: (wrap lines at 72 characters)
Move buggy bootstrap-icons dependency to core/package.json


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

@damithc
Copy link
Contributor

damithc commented Apr 30, 2024

Thanks for the quick fix @kaixin-hc

Copy link
Contributor

@EltonGohJH EltonGohJH left a comment

Choose a reason for hiding this comment

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

I think this should fix the problem even if it dun it is still a good idea to shift it anyway.

So, we can merge it in!

@yiwen101
Copy link
Contributor

LGTM
Thank you for the quick investigation and fix

@yiwen101 yiwen101 merged commit 149244f into MarkBind:master Apr 30, 2024
10 checks passed
@github-actions github-actions bot added the r.Patch Version resolver: increment by 0.0.1 label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r.Patch Version resolver: increment by 0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants