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

Fix 8141: fix npm run build sass icon failure which prevents types from being built #8217

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zorent-zebra
Copy link

@zorent-zebra zorent-zebra commented Mar 29, 2023

Description

Attempt to fix angular not building in video.js version 8 reported in #8141

Specific Changes proposed

sass was erroring out for me when running npm run build on the steps build:css:cdn and build:css:default.
This seemed to cause the build not to get to build:types.
In my local 8.2.1 video.js in angular, there is no file ./dist/types/video.d.ts.
My guess is that it's due to this build failure.

I haven't checked whether this affects building using yarn....
We don't use yarn.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Mar 29, 2023

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@misteroneill misteroneill added the ts TypeScript label Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #8217 (cec7a5b) into main (882f3af) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #8217   +/-   ##
=======================================
  Coverage   82.11%   82.11%           
=======================================
  Files         112      112           
  Lines        7402     7402           
  Branches     1785     1785           
=======================================
  Hits         6078     6078           
  Misses       1324     1324           

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

@LexanRed
Copy link

Is there any update on this one?

@zorent-zebra
Copy link
Author

Alas I have reason to believe that this PR won't fix the issue.
If I have time, will try to research more.

@misteroneill misteroneill marked this pull request as draft June 6, 2023 20:04
@misteroneill
Copy link
Member

Hey @zorent-zebra thanks for the PR!

I've converted this to a draft based on the last comment. Feel free to update when you can and mark it ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants