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

A fifth composite pull request #81

Merged
merged 3 commits into from Nov 12, 2019
Merged

A fifth composite pull request #81

merged 3 commits into from Nov 12, 2019

Conversation

hmdne
Copy link
Member

@hmdne hmdne commented Oct 16, 2019

Can we clone a DOM node? Now we can!

This is a fifth composite pull request that encompasses my ongoing work on opal-browser.

Those are two separate, albeit somehow related features.

Node#document=, as name suggests, changes a document of some node
using Document.adoptNode. The node is first detached from an old
document.

Note#initialize_copy allows you to #clone or #dup your Nodes by
cloning a DOM node as well. This feature fixes opal#65.

Implementing the second one required some minor integration with
NativeCachedWrapper. I used a long name for a method changing the
native reference variable: `set_native_reference`, kind of mirroring
the use of `instance_variable_set`, with a similar semantic: "Don't
use this method unless you really know what you are doing".

This new feature may somehow conflict with how we use some instance
variables: #clone/#dup copies them, but well. It's better to have a
standard Ruby way to clone a DOM node than to not have it. There
shouldn't be too many bugs, maybe some edge cases with the animation
subsystem. We can iron that out later.
This will allow us to compute SHA-family hashes of Buffers (ArrayBuffers),
for example Buffers that can be extracted from Files.
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I missed this completely!
Great work 👍

BTW I don't remember if I already told you, but I've added you as a triage/collaborator, that should give you enough permissions to set labels, assign issues and PRs and stuff like that. If you think would make sense for you to have more permissions let's talk on gitter or via mail 👍

@elia elia merged commit 18a4087 into opal:master Nov 12, 2019
@AlexWayfer
Copy link

Thank you!

@hmdne
Copy link
Member Author

hmdne commented Nov 14, 2019

@elia Thank you for permissions, they will be useful. I will tidy up the issues right now.

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

3 participants