-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature animation controller #45
Conversation
…ash modal window on close event
…wn-ccv/VR-Volumeviewer into feature-animation-controller
…wn-ccv/VR-Volumeviewer into feature-animation-controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great a lot of work! My comments are mostly formatting. It would be nice if you can start heading towards smaller PRs, that would make it a bit easier to follow the logic changes as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Lots of good work going on here. I think the main things I've seen are
- Delete commented out code
- Fix tab indenting
- Remove unnecessary extra lines
snake_case
vsTitleCase
vscamelCase
consistency- Lots of
if
statements that can be consolidated to one line (or 2 without the{ }
if you prefer - I think a couple comments in places where the code gets super busy would go a long way - Mary pointed out some other ones as well
… renaming variables to camel notation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great progress!! I still see quite a few older unresolved comments from both myself and @RobertGemmaJr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor nits and @RobertGemmaJr still has some unresolved comments to look at, but let's get this merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I'd just do a clean sweep of any unresolved convos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the consistency in formatting!
This PR Includes: