-
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): initial support for Observations #4631
Conversation
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
As promised
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.
In this PR you do to much outside of the architecture. Our observation should be in the domain object itself and added to the root at startup.
react-components/src/architecture/concrete/observationsDomainObject/ObservationsView.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
react-components/src/architecture/concrete/observationsDomainObject/ObservationsTool.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/observationsDomainObject/ObservationsDomainObject.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/exampleDomainObject/ExampleView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/primitives/box/BoxView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/primitives/line/LineView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/primitives/plane/PlaneView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/terrainDomainObject/TerrainThreeView.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.
LGTM. Some few comments but they are not blockers. Good job!
Note: Let's wait Nils reviews as he can see some more pitfalls that I'm not seeing right now since he knows this architecture better atm.
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 here is things you still have misunderstood. Having a hard reference to a DomainObject directly in a tool wrong. I said in the course that number of references to a object should be minimum and controlled by the architecture. What happens for instance when another command delete it, but the reference is still in the tool? The architecture will guarantee that the domain objects hold by the root (as a hierarchy) is "alive".
react-components/src/architecture/concrete/observationsDomainObject/ObservationsDomainObject.ts
Show resolved
Hide resolved
react-components/src/architecture/concrete/exampleDomainObject/ExampleView.ts
Outdated
Show resolved
Hide resolved
react-components/src/architecture/concrete/observationsDomainObject/ObservationsDomainObject.ts
Show resolved
Hide resolved
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
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
Type of change
Jira ticket 📘
Description 📝
Initial support for observations. Includes a couple of other changes.
BaseView
class, to make it explicit what DomainObject it concerns.useMemo
to avoid recreating command objects repeatedly in React command buttonsfilter
in theinstanceFilter
query is made undefined if empty (otherwise it's an invalid request), as was done with thesearch
endpoint earlier.How has this been tested? 🔍
In storybook (Toolbar) and linked into Fusion
Test instructions ℹ️
Checklist ☑️