-
Notifications
You must be signed in to change notification settings - Fork 0
Composer Module #2
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
base: master
Are you sure you want to change the base?
Conversation
|
Main text was drafted, now it would be nice to have some feedback. /cc @RyuuGan @baruchvlz |
baruchvlz
left a comment
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, just a few pointers.
| ### `ComposerPreviewComponent` | ||
|
|
||
| - Selector: `orc-composer-preview` | ||
| - It will render current json configuration via `orc-orchestrator` |
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.
We could use something like hightlight.js to display the JSON
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 think it's a good idea, it will be also useful for editing functions when configuring components.
For that matter I think monaco editor will be more suitable, it's what vscode is built on.
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 feature will allow to provide autocomplete in functions when writing them
accepted/0000-composer-module.md
Outdated
| ## Unresolved Questions and Bikeshedding | ||
|
|
||
| - We need to agree how json configuration will be exposed to user. It can be done via output that is triggered when user clicks on `Generate` button or configuration is emitted every time it's changed. | ||
| This can also be configurable via DI token. |
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 think we should just display the JSON format and update it on every change.
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.
Yeap, as I wrote we can have an additional component for this purpose.
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.
We don't need an additional component. We can use this in the ComposerPreviewComponent
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.
Actually you are right, we can use both errors and json components in ComposerPreviewComponent, because it is connected with preview.
| - It should render `RenderItemComponent` inside with `ComposerDroppableComponent` in appropriate places | ||
| - When configuration is updated it should rearrange position of `ComposerDroppableComponent` | ||
|
|
||
| ### `ComposerPreviewComponent` |
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.
Component should have a download button and a copy button so the user can do either action faster.
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.
Aggreed with duplicate button.
RyuuGan
left a comment
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.
Everything looks good, but I would add some changes
|
|
||
| - `<orc-composer-components>` - will render list of all available components ready for dragging | ||
| - `<orc-composer-canvas>` - will render area for dropping components and configure them | ||
| - `<orc-composer-preview>` - will render currently configured components in live preview |
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.
Also I would add 2 more components:
<orc-composer-errors>- will render list of errors that were while rendering<orc-composer-json>- will render result json, aslo it should be possible to edit the whole config via this json. So when user clicks to download button, we can show this result and copy everything to clipboard.
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.
Both of them can be part of <orc-composer-preview> but named differently and used internally in it.
accepted/0000-composer-module.md
Outdated
|
|
||
| Communication between components may happen via inputs/outputs as well as via parent `ComposerComponent`. | ||
|
|
||
| Any errors during live preview of configuration must be rendered in toast. |
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.
As I wrote before errors can be also rendered to <orc-composer-errors>, so user will know all the errors that happened while renderening. Toast will finally disapper so it is not that reliable.
| - It should render `RenderItemComponent` inside with `ComposerDroppableComponent` in appropriate places | ||
| - When configuration is updated it should rearrange position of `ComposerDroppableComponent` | ||
|
|
||
| ### `ComposerPreviewComponent` |
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.
Aggreed with duplicate button.
accepted/0000-composer-module.md
Outdated
| ## Unresolved Questions and Bikeshedding | ||
|
|
||
| - We need to agree how json configuration will be exposed to user. It can be done via output that is triggered when user clicks on `Generate` button or configuration is emitted every time it's changed. | ||
| This can also be configurable via DI token. |
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.
Yeap, as I wrote we can have an additional component for this purpose.
|
I've updated RFC with received feedback |
baruchvlz
left a comment
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
RyuuGan
left a comment
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.
Added fix for small typo. Also added changes that will add Component to all components, otherwise it is a little bit different: some components are named with Component in the end, other not.
Anything else is fine.
Co-Authored-By: gund <malkevich.alex@gmail.com>
|
Thanks @RyuuGan - I've applied your fixes |
RyuuGan
left a comment
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
Co-authored-by: Valery Aligorsky <stdammit@gmail.com> re orchestratora/rfcs#2
Co-Authored-By: Valery Aligorsky <stdammit@gmail.com> re orchestratora/rfcs#2
Co-Authored-By: RyuuGan <stdammit@gmail.com> re orchestratora/rfcs#2
re orchestratora/rfcs#2 Co-Authored-By: RyuuGan <stdammit@gmail.com>
So they can be extended via DI from third-party modules re orchestratora/rfcs#2
re orchestratora/rfcs#2 Co-Authored-By: RyuuGan <stdammit@gmail.com>
So they can be extended via DI from third-party modules re orchestratora/rfcs#2
RFC text