Skip to content

fix: Refactor DOMElement#266

Open
alexanderGugel wants to merge 5 commits into
Famous:developfrom
alexanderGugel:rewrite-DOMElement
Open

fix: Refactor DOMElement#266
alexanderGugel wants to merge 5 commits into
Famous:developfrom
alexanderGugel:rewrite-DOMElement

Conversation

@alexanderGugel

Copy link
Copy Markdown
Member
  • No longer keep track of unnecessary state (e.g. uiEvents)
  • Fix onMount
  • Allow adding the DOMElement via Node#addComponent instead of DOMElement
    constructor
  • Improve (basically rewrite) test suite
  • Allow recycling of DOMElement
  • Store state in external Spec (consistent with Node)
  • Make DOMElement#getValue consistent with spec naming conventions
    (e.g. styles/ id)
  • Add missing getter functions
  • Fix documentation
  • Obsolete/ Remove DOMElement#draw DOMElement#init (users called it
    explicitly due to confusion)
  • Fix return values described in the docs (make methods chainable)
  • Properly reset ElementCache when elements are being reused
  • Generalize dismounting: preserve DOMElement spec
  • Reset properties, attributes etc. onDismount

Simple example of one of the things that was broken:

var X = 10;
var Y = 10;

for (var x = 0; x < X; x++) {
    for (var y = 0; y < Y; y++) {
        var div = new DOMNode({
            properties: {
                background: 'red'
            }
        });

        div.setSizeMode('relative', 'relative');
        div.setProportionalSize(1/X, 1/Y);
        div.setOpacity(0.1);
        div.setAlign(x/X, y/Y);

        div.setRotation(0, 0, Math.PI*0.1);

        scene.addChild(div);

        setTimeout(function(div) {
            scene.removeChild(div);
            setTimeout(function(div) {
                scene.addChild(div);
                div.el.setProperty('background', 'blue');
            }.bind(null, div), 100);
        }.bind(null, div), 1000);
    }
}

Before: https://api-te.famo.us/codemanager/v1/containers/fe4b52ef-9d0b-4f71-9368-e52d22625c6b/share

A lot of stuff was just broken.

Please review @DnMllr

@alexanderGugel alexanderGugel force-pushed the rewrite-DOMElement branch 3 times, most recently from bf00479 to 4ec3462 Compare June 11, 2015 12:16
@alexanderGugel alexanderGugel force-pushed the rewrite-DOMElement branch 2 times, most recently from c16cece to b4f5e59 Compare June 12, 2015 07:46
@alexanderGugel

Copy link
Copy Markdown
Member Author

@michaelobriena What can I do to make this less controversial? E.g. I didn't change any user facing APIs. Is there anything else here that would need to be resolved?

@alexanderGugel

Copy link
Copy Markdown
Member Author

This is still something that really needs to happen IMO, since the DOMElement in its current state doesn't handle a lot of cases correctly and there is also no reason why users shouldn't be able to do node.addComponent(new DOMElement). This PR fixes a lot of those case, but conflicts with feature-systems. Rebasing.

@alexanderGugel alexanderGugel force-pushed the rewrite-DOMElement branch 2 times, most recently from 575a3f1 to 3c2d336 Compare June 19, 2015 13:30
@alexanderGugel

Copy link
Copy Markdown
Member Author

I uncommented the tests for now. I still need to adjust them to use the Node.stub and TransformSystem.stub, but I don't think this presents a regression, since most of the tests have been commented out for feature-systems anyways.

@alexanderGugel

Copy link
Copy Markdown
Member Author

There are some nasty merge conflicts due to feature-systems. I'm rebasing this right now.

@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr Thoughts?

@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr Could you review this, please?

@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr Any thoughts on this?

* No longer keep track of unnecessary state (e.g. uiEvents)
* Fix onMount
* Allow adding the DOMElement via Node#addComponent instead of DOMElement
  constructor
* Improve (basically rewrite) test suite
* Allow recycling of DOMElement
* Store state in external Spec (consistent with Node)
* Make DOMElement#getValue consistent with spec naming conventions
  (e.g. styles/ id)
* Add missing getter functions
* Fix documentation
* Obsolete/ Remove DOMElement#draw DOMElement#init (users called it
  explicitly due to confusion)
* Fix return values described in the docs (make methods chainable)
* Properly reset ElementCache when elements are being reused
* Generalize dismounting: preserve DOMElement spec
* Reset properties, attributes etc. onDismount
@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr Could you please review this if possible? Thanks.

@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr I rebased this a couple of times now. I'm just gonna leave this open for now and wait for the review.

@alexanderGugel

Copy link
Copy Markdown
Member Author

@DnMllr Could you have a look over/ review this? Any thoughts?

@alexanderGugel

Copy link
Copy Markdown
Member Author

@michaelobriena
I'm just going to leave this open until someone finds the time to review this.

update The implementation of render size in the DOMElement changed in the mean time. I'm looking into it right now.

@trusktr

trusktr commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

Please, please, please, can you guys merge this sometime soon?

@michaelobriena @alexanderGugel @marklu

@trusktr

trusktr commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

Hmmm, I have an idea: I'll merge this into http://github.com/infamous/engine and test it, then publish that on NPM (infamous-engine) if it fixes things.

cc @talves @steveblue @gadicc

@trusktr

trusktr commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

The community and I would greatly appreciate if you guys spent all your time right now fixing bugs, not making new features, not making new API changes, not planning other libraries or frameworks. Just fixing all the bugs we have. It doesn't make sense to move forward with a boatload of bugs at hand.

@jd-carroll

Copy link
Copy Markdown
Contributor

This PR works well

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants