-
Notifications
You must be signed in to change notification settings - Fork 20
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(react-components): create and delete observations #4658
Conversation
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 had some comments
react-components/src/architecture/concrete/observationsDomainObject/ObservationsDomainObject.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observationsDomainObject/ObservationsTool.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observationsDomainObject/ObservationsTool.ts
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.
See 2 new comments
react-components/src/architecture/concrete/observationsDomainObject/ObservationsDomainObject.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observationsDomainObject/ObservationsView.ts
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.
Better now, but still a way to go. I hope you understand 'why' I'm stressing these things. The point is to follow the pattern, then everyone will understand your code and we will spend less time in bug fixing and additional features.
Also, I strongly want to remove the _fdmSdk in the ObservationsCache, since this can be used as a parameter instead for save and delete. You have access to renderTarget almost everywhere in the code, so double buffering of this pointer make more complexity.
react-components/src/architecture/concrete/observations/ObservationsDomainObject.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsTool.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsView.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/SaveObservationsCommand.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/types.ts
Outdated
Show resolved
Hide resolved
I can do this, but the FdmSDK is still used in the constructor to load the initial set of observations, which means it must still be sent to the ObservationsCache on creation. (Edit: I made the change now) |
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 very close to "perfect" now. I had some comment. Not all of them is important,
react-components/src/architecture/concrete/observations/ObservationsCache.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsDomainObject.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsDomainObject.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsDomainObject.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsTool.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsView.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsView.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/ObservationsView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observations/SaveObservationsCommand.ts
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.
Looks good now
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/BND3D-4179
https://cognitedata.atlassian.net/browse/BND3D-4180
(Note that the above tasks should also have more involved UI for observation handling, which is not addressed by this PR
Description 📝
Buttons to create and delete observations, and committing the changes to the cloud.
How has this been tested? 🔍
In storybook and Search/Fusion
Test instructions ℹ️
{ modelId: 3544114490298106, revisionId: 6405404576933316 }
(you can change directly from id-1
,-1
ingetAddModelOptionsFromUrl.ts
)Checklist ☑️