-
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
(feat) Architecture on top of Reveal #4403
Conversation
This reverts commit f09ef09.
…/cognitedata/reveal into np/actitecture-in-reactcomponent
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 believe this contribution will be a major cornerstone in future development, and I'm conceptually very happy about it. There are some conventions/code style choices that disrupt our current norms, but I think we should wait until we've gotten our hands wet in testing it out before addressing it and coming up with better suggestions. I'll note some of my immediate concerns here:
- The current setup treats constructions as multiple-statement processes, so that e.g. constructing a domain object often looks like this:
const object = new DerivedDomainObject();
object.field = value;
....
I'd rather put such field initializations into the constructor itself
- Users are expected to inherit from e.g. Domain objects to integrate with the architecture, and there is in general heavy emphasis on class hierarchies and inheretance throughout the architecture. Obviously, it will work, but I do wonder if we can get away with something more "Typescript-y". It also means that many fields (name, color etc.) will be present in all Domain Objects even though they may not be relevant for all (at least unless we have a 2D three-view of the DomainObject hierarchy, of which there has been some talk. I believe @christjt had some ideas around this involving composition. I'll allow him to describe his ideas himself when he's back
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-ish 👍
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
Overview of the proposed architecture her:
https://docs.google.com/presentation/d/1Y50PeqoCS5BdWyDqRNNazeISy1SYTcx4QDcjeMKIm78/edit?usp=sharing
There will also be documentation in the Readme.md file:
src\architecture\base\README.md
(WORK ON THIS NOW)Link here: https://github.com/cognitedata/reveal/blob/np/actitecture-in-reactcomponent/react-components/src/architecture/base/README.md
How has this been tested? 🔍
This will goto: https://localhost:3000/?modelUrl=primitives
Open Architecture/Main
The toolbar to the right hand side contains all new functionality
Checklist ☑️