Minor changes to REMOVE_SCENE/ core #427
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Should fix #330. The corresponding functionality wasn't implemented. Probably needs some tests just to be sure. |
alexanderGugel
force-pushed
the
REMOVE_SCENE-command
branch
2 times, most recently
from
9b9035c
to
dc5df3b
Compare
| @@ -402,12 +405,13 @@ FamousEngine.prototype.addScene = function addScene (scene) { | |||
| * @return {FamousEngine} this | |||
| */ | |||
| FamousEngine.prototype.removeScene = function removeScene (scene) { | |||
| var selector = scene._selector; | |||
| var selector = scene._id; | |||
There was a problem hiding this comment.
What is the difference between _id and _selector?
There was a problem hiding this comment.
Scene now inherits from Node. This significantly simplifies the Scene constructor. The id of a scene is its selector, therefore it should be able to be retrieved through getLocation etc.
The `FamousEngine` is supposed to expose an API for adding and removing
scenes. While the corresponding methods have been outlined, the Scene
class itself doesn't reflect the ability to re-mount the scene to a
different selector.
* The Scene constructor accepts an updater and a selector
* Scene#_globalUpdater is inconsistent with the inherited,
Node-specific (theoretically) Node#_updater
* The Scene updater is being set as via the constructor (instead of
Node#_setUpdater)
* The Scene constructor requests the context size
* The scene constructor no longer accepts any arguments
* The scene itself is not bound to a specific selector, but instead
can be mounted to an arbitrary path (just like nodes)
* Scene constructor logic has been moved to Scene#mount
* Less redundancy
* More flexibility: Scene can request size at a later point in time
instead of upon instantiation
* Scene constructor does not have side effects
* Scene updater (Scene#_globalUpdater) can be changed via Dispatch
* => FamousEngine#removeScene and FamousEngine#addScene can be
implemented
alexanderGugel
force-pushed
the
REMOVE_SCENE-command
branch
from
dc5df3b
to
d35c27b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses multiple issues with the current Scene implementation (especially see 20a6744).