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

Vexflow migration 4.0.0 (old packages) #1085

Closed

Conversation

rvilarl
Copy link
Contributor

@rvilarl rvilarl commented Nov 10, 2021

No description provided.

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

Hi Simon, I have reverted to the old packages as there were some issues with the new ones and as you said, let us keep the issues separated.

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

This integration is complete with all checks passed!

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

@sschmidTU I would say that there is some problem with ties, can you have a look at the demo? Thanks

@sschmidTU
Copy link
Contributor

sschmidTU commented Nov 10, 2021

As said in #1083, please remove all the refactors that change the return type from Node to void.
I'll take a look at the ties.
Maybe we do want the new webpack to improve the build process, though it seems to compile for now. But yeah, a few packages will be problematic to update, so i appreciate not updating all of them at once.

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

As said in #1083, please remove all the refactors that change the return type from Node to void.

I made that because I saw that svg was returning Node and canvas undefined and I thought that if it works with undefined it was better to unify.

Maybe we do want the new webpack to improve the build process, though it seems to compile for now. But yeah, a few packages will be problematic to update, so i appreciate not updating all of them at once.

I took out the packages changes based on your comment and also based on the fact that I awas getting the same error as in #1059:

✖ multiple instances

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

Please confirm if you still want the Node change reverted and if this is the case, I will do it.

@sschmidTU
Copy link
Contributor

sschmidTU commented Nov 10, 2021

I made that because I saw that svg was returning Node and canvas undefined and I thought that if it works with undefined it was better to unify.

Canvas can't return a node because it doesn't have a DOM.
Accordingly, you can't precisely delete something you drew on a canvas if it's overlapping something else, as far as i know.

Please confirm if you still want the Node change reverted and if this is the case, I will do it.

Yes, please revert it.

@rvilarl
Copy link
Contributor Author

rvilarl commented Nov 10, 2021

Done

}
this.CanvasRenderingCtx.globalAlpha = alpha;
this.ctx.fillRect(rectangle.x, rectangle.y, rectangle.width, rectangle.height);
this.CanvasRenderingCtx.fillStyle = old;
this.CanvasRenderingCtx.globalAlpha = 1;
return undefined; // can't return dom node like with SVG
return undefined; // can't return svg dom node
Copy link
Contributor

@sschmidTU sschmidTU Nov 10, 2021

Choose a reason for hiding this comment

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

please revert this comment change. the previous one is clearer. of course you can't return an svg node in canvas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only this instance or make the same comment on the other instances within CanvasVexFlowBackend? They should all be the same within this file, should not them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will unify to the clearer one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@sschmidTU
Copy link
Contributor

sschmidTU commented Nov 10, 2021

It looks like Github is having some issues with comments on PRs, i can't reply to some of them anymore at some point, and sometimes it shows the comment belonging to the wrong code line.

Responding to:
image
You only need to revert this one comment change. I don't see where you could make a similar change either.

edit: now i see it in your latest commit, it's good to revert them all, thank you!

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.

None yet

2 participants