Skip to content

Conversation

@Puneethnaik
Copy link
Contributor

#2737 issue fix. @Tyriar Please look into this. Would love to hear feedback.

…3 to the selection color if selection color provided is opaque.
…from ColorManager to a function in color namespace.

2. Changed the DomRenderer and CanvasRenderer where the function defined in color namespace is called.
@Puneethnaik
Copy link
Contributor Author

@Tyriar Please consider the changes. I observed some checks had failed.(Particularly the integration tests). I have committed few changes that passes all the checks. I moved logic from ColorManager setTheme function to a function in color namespace in Color.ts. Also, I invoke the function in DomRenderer and Renderer class constructor. Would love to hear feedback.

@Tyriar
Copy link
Member

Tyriar commented Jun 3, 2020

@Puneethnaik I'll check this out next week, after a quick look at the code it looks pretty much like what I had in mind 👍

@Tyriar Tyriar added this to the 4.7.0 milestone Jun 3, 2020
@Tyriar Tyriar self-assigned this Jun 3, 2020
@Puneethnaik
Copy link
Contributor Author

@Tyriar thanks for considering the PR. Since this is my first PR in xterm.js, I am very excited. Will wait for the review and any feedback 👍. Meanwhile I will keep studying the codebase, learnt a lot.

@Tyriar
Copy link
Member

Tyriar commented Jun 4, 2020

@Puneethnaik good to hear 🙂, there's always more help wanted issues if you're keen.

@Puneethnaik
Copy link
Contributor Author

@Tyriar Thank you for the feedback. I fixed the code while consulting the feedback and pushed a commit. However, the checks are not passing. Upon some investigation, it seems a 404 Not found error occured, When I looked at the Azure devops log. I checked under linux tests section. There the message contained url: 'https://github.com/tonsky/FiraCode/raw/master/distr/otf/FiraCode-Regular.otf',
Upon following the link, it gave a 404. I checked the recent commit of FiraCode on the master branch(link here it seems that FiraCode-Regular.otf is removed in this commit.
Please guide me on this.

@Tyriar
Copy link
Member

Tyriar commented Jun 11, 2020

The font CI problem should get fixed with #2971

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for the help with this @Puneethnaik! 🚀

@Tyriar Tyriar merged commit ac0e1ae into xtermjs:master Jun 11, 2020
@Puneethnaik
Copy link
Contributor Author

Thank you @Tyriar. It is really an honor to start my open source track record by contributing to a great product like xterm.js. I will keep looking at more issues and try to contribute more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants