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

feat(model3d): Add flyout for animation clips #1369

Merged

Conversation

ConradJChan
Copy link
Contributor

@ConradJChan ConradJChan commented May 4, 2021

TODO:

  • unit tests

Before:
Screen Shot 2021-05-03 at 9 57 20 PM

After:
Screen Shot 2021-05-03 at 9 57 50 PM

@ConradJChan ConradJChan force-pushed the react-model3d-animation-controls branch from 7da08c9 to 240fc99 Compare May 4, 2021 21:42
@ConradJChan ConradJChan marked this pull request as ready for review May 4, 2021 23:25
@ConradJChan ConradJChan requested a review from a team as a code owner May 4, 2021 23:25
src/lib/viewers/box3d/model3d/Model3DViewer.js Outdated Show resolved Hide resolved
src/lib/viewers/controls/radioitem/RadioItem.tsx Outdated Show resolved Hide resolved
src/lib/viewers/box3d/model3d/Model3DViewer.js Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/SettingsFlyout.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/model3d/AnimationClipsControl.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/model3d/AnimationClipsControl.tsx Outdated Show resolved Hide resolved
return x.length >= width ? x : new Array(width - x.length + 1).join('0') + x;
};

export const formatDurationStr = (duration: number): string => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have something similar to this in the DurationLabels component?

Copy link
Contributor Author

@ConradJChan ConradJChan May 5, 2021

Choose a reason for hiding this comment

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

Yeah you're right, I just copied this over from the original implementation. I'll move the DurationLabels implementation into a controls/utils.ts file and import it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On further investigation, seems these two serve different functions (although similar). The DurationLabels method will strip off the hours if it is zero, but this function wants to always display hh:mm:ss so unless you have a strong objection, I'll keep this one here

src/lib/viewers/controls/model3d/AnimationClipsControl.tsx Outdated Show resolved Hide resolved

describe('icon prop', () => {
// eslint-disable-next-line @typescript-eslint/no-unused-vars
function CustomIcon({ isOpen, ...rest }: object, ref: React.Ref<HTMLDivElement>): JSX.Element {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to destructure isOpen out here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to avoid a console error while running unit tests since isOpen isn't a valid attribute on a div

@@ -11,9 +11,16 @@ import { decodeKeydown } from '../../../util';

export type Props = React.PropsWithChildren<{
className?: string;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
icon?: React.ComponentType<any>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was fighting this for a while but couldn't get it to work. Open to suggestions

@ConradJChan ConradJChan force-pushed the react-model3d-animation-controls branch 2 times, most recently from 1408979 to ce67531 Compare May 5, 2021 18:40
@ConradJChan ConradJChan force-pushed the react-model3d-animation-controls branch from ce67531 to e6f56f6 Compare May 5, 2021 18:44
src/lib/viewers/box3d/model3d/Model3DRenderer.js Outdated Show resolved Hide resolved
<AnimationClipsToggle />
{/* TODO: AnimationClipsFlyout */}
</>
<Settings className="bp-AnimationClipsControl" icon={AnimationClipsToggle}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be toggle, as well?

src/lib/viewers/controls/settings/Settings.tsx Outdated Show resolved Hide resolved
src/lib/viewers/controls/settings/Settings.tsx Outdated Show resolved Hide resolved
@ConradJChan ConradJChan merged commit 09eefd9 into box:master May 5, 2021
@ConradJChan ConradJChan deleted the react-model3d-animation-controls branch May 5, 2021 21:11
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.

2 participants