-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore(react-components): remove model containers #4448
base: master
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
function styleAnnotation( |
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.
Any reason to have this function outside the class
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.
More of a personal preference - if a function doesn't need (or change) the state of a class, I put it outside. It makes it conceptually easier to reason about, since you know all its inputs. I can move it if you disagree strongly
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
const addedModel = await (async () => { | ||
if (is3dModelOptions(addOptions)) { | ||
const addedModel = await this._viewer.addModel(addOptions); | ||
if (addedModel instanceof CogniteCadModel) { |
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.
It would be nice to have a function isCadModel()
and ìsPointCloudodel()
and reuse the across react-components
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 could just create a function that uses instanceof
inside, but I think it's an unnecessary abstraction. I can do it if you feel strongly about it
react-components/src/components/Reveal3DResources/ResourceUpdater.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/applyPointCloudStyling.ts
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculateCadModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculatePointCloudModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculatePointCloudModelsStyling.tsx
Outdated
Show resolved
Hide resolved
react-components/src/components/Reveal3DResources/calculatePointCloudModelsStyling.tsx
Outdated
Show resolved
Hide resolved
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.
Some comments on first iteration, haven't yet tested it.
Will take second review!
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/BND3D-4226
Description 📝
How has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️