Skip to content
This repository has been archived by the owner on Apr 25, 2018. It is now read-only.

Clarify types and enable flow for node proxies #92

Merged
merged 1 commit into from
May 3, 2016

Conversation

brandonpayton
Copy link
Contributor

Hi @garbles,

I was happy to see the addition of VirtualText, noticed your commit about disabling flow in NodeProxy, and decided to take a look at the flow issue. I am wondering whether you will want to keep a single proxy type, but I ended up thinking we should have separate proxy types for element and text because they are simply different things.

Here's a summary of my changes:

  • Split NodeProxy into ElementProxy and TextProxy
  • Enabled flow for both proxy modules
  • Set text node content by setting nodeValue rather than textContent because it is more direct
  • Added unit tests for TextProxy (just 2 test cases)
  • Renamed VirtualNode to VirtualElement for clarity because both text and elements are nodes in the DOM.
  • Renamed the union VirtualElement to VirtualNode in types.js
  • Renamed NodeProxyDecorator to ElementProxyDecorator in types.js

If you are open to these changes and how I wrote the TextProxy tests, I can also write tests for ElementProxy either before or after landing this.

What do you think?

@garbles
Copy link
Owner

garbles commented May 3, 2016

LGTM. Thanks so much for doing this!

@garbles garbles merged commit 73f8978 into garbles:master May 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants