-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Merge ligatures addon into core repo #2847
Conversation
b93cf82
to
9af1847
Compare
Looks pretty good, tests even run and pass 😃 I'll give it a proper review as soon as I can |
3562446
to
feab7c2
Compare
I was trying to add a webpack config similar to other addons, but getting errors for |
Yes it can only be used in node/electron as it pulls fonts from disk. |
added webpack config similar to other addons |
@Tyriar the ci on windows is failing because of |
@LabhanshAgrawal that's odd, pretty sure the same thing is run for the other addons so I don't know what the problem could be. Unless the addons don't actually get built on other platform in which case we should fix that generally before merging. If that is the case could you create an issue? |
@Tyriar While doing |
d911848
to
14ad58f
Compare
@Tyriar please review. |
154794c
to
5d4c4d7
Compare
rebase and pushed |
This week I'm focusing on testing VS Code, next week I'll be merging in xterm.js PRs and releasing 4.6. I expect this should be a pretty easy merge then 🙂 |
5d4c4d7
to
ad9aabb
Compare
I notice the addon doesn't yet depend on the core codebase so breakages could still happen. We should write integration test(s) so breakages are detected early #2896 |
Had an idea which could maybe do away with a lot of the complexity and native access in this addon #2897 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! It should be much easier to maintain this now, especially after some integration tests are added #2896
Will you be moving the issues from the old repo? if that's possible. |
Done thanks, and archived the old one. |
See discussions here for context: vercel/hyper#3607 (comment) xtermjs/xterm.js#3286 xtermjs/xterm.js#2847 vercel/hyper#5888
Fixes #2726
@Tyriar see if this seems ok (first time contributing to xterm)