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

warning displays - renamed/deleted nodes in the inherited scene file #52

Merged
merged 13 commits into from Jul 13, 2018

Conversation

tony056
Copy link
Contributor

@tony056 tony056 commented Jul 12, 2018

  • Changes:
    • added SnackBar.js to display different types of conflicts.
    • checked missing nodes in SceneLoader.js.
    • changed css styles for ui tree nodes.
    • added warning snackbar rendering in HeirarchyPanelContainer.js.

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

Mostly minor comments. Using userData and properties starting with underscores is important though, since we will be using that later to filter out unwanted properties.

<path id="Path_1" data-name="Path 1" class="cls-1" d="M8,16a8,8,0,1,1,8-8,8,8,0,0,1-8,8ZM8,2a6,6,0,1,0,6,6A6,6,0,0,0,8,2Z"/>
<path id="Path_2" data-name="Path 2" class="cls-1" d="M8,7A1,1,0,0,0,7,8v3a1,1,0,0,0,2,0V8A1,1,0,0,0,8,7Z"/>
<circle id="Ellipse_1" data-name="Ellipse 1" class="cls-1" cx="1.188" cy="1.188" r="1.188" transform="translate(6.812 3.812)"/>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these icons from? We we need to provide attribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assets are from here: https://design.firefox.com/icons/viewer/
It's open for everyone to download. I didn't see anything about attribution, but I might missed it. Let me know if you see something about attribution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, cool. Um, I suppose technically these are licensed under MPL, but I think we're good since it's internal.

parentObject = new THREE.Object3D();
parentObject.name = entity.parent;
parentObject.isMissingRoot = true;
parentObject.missing = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

isMissingRoot and missing should be on parentObject.userData and should start with an underscore.

duplicate: {
img: warningIcon,
content: "Duplicate node names in your gltf modals.",
helpURL: "https://github.com/MozillaReality/hubs-editor/wiki/Tutorials"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just create a couple of placeholder wiki pages now.

})}
onMouseUp={e => this.onMouseUpNode(e, node)}
onMouseUp={node.object.missing ? undefined : e => this.onMouseUpNode(e, node)}
onMouseDown={node.object.missing ? e => e.stopPropagation() : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, would null work here instead of undefined? I think we prefer using null in code.

scene.conflicts = {
missing: false,
duplicate: false
};
Copy link
Contributor

Choose a reason for hiding this comment

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

conflicts should be under scene.userData and should start with an underscore.

return;
}
const conflicts = this.props.editor.scene.conflicts;
console.log(Object.keys(conflicts));
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the console.log

align-content: space-between;
flex-wrap: wrap;
position: absolute;
width: 80%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm not sure about using percentages. I think we use fixed pixels everywhere else.

const conflictTypes = {
missing: {
img: errorIcon,
content: "Missing nodes in your .scene file.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be "Missing nodes in inherited GLTF"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah, my bad.

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

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

LGTM!

@tony056 tony056 merged commit ffabcc6 into master Jul 13, 2018
@tony056 tony056 deleted the features/warning-displays branch July 13, 2018 22:52
robertlong pushed a commit that referenced this pull request Aug 13, 2018
warning displays - renamed/deleted nodes in the inherited scene file
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