Skip to content
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

Composer Module #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Composer Module #2

wants to merge 4 commits into from

Conversation

gund
Copy link
Member

@gund gund commented Mar 7, 2019

@gund gund added the enhancement New feature or request label Mar 7, 2019
@gund
Copy link
Member Author

gund commented Mar 8, 2019

Main text was drafted, now it would be nice to have some feedback.

/cc @RyuuGan @baruchvlz

Copy link

@baruchvlz baruchvlz left a 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`

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

Copy link
Member Author

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.

Copy link
Member Author

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

## 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.

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.

Copy link

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.

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

Copy link

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`

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aggreed with duplicate button.

Copy link

@RyuuGan RyuuGan left a 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
Copy link

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.

Copy link

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.


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.
Copy link

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`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aggreed with duplicate button.

## 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.
Copy link

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.

@gund gund requested review from RyuuGan and baruchvlz March 10, 2019 18:22
@gund
Copy link
Member Author

gund commented Mar 10, 2019

I've updated RFC with received feedback

Copy link

@baruchvlz baruchvlz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@RyuuGan RyuuGan left a 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.

accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
accepted/0000-composer-module.md Outdated Show resolved Hide resolved
@gund
Copy link
Member Author

gund commented Mar 11, 2019

Thanks @RyuuGan - I've applied your fixes

Copy link

@RyuuGan RyuuGan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gund added a commit to orchestratora/orchestrator that referenced this pull request Mar 15, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 15, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 15, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 15, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 15, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 16, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jun 16, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Aug 6, 2019
gund added a commit to orchestratora/orchestrator that referenced this pull request Jan 15, 2022
gund added a commit to orchestratora/orchestrator that referenced this pull request Jan 15, 2022
gund added a commit to orchestratora/orchestrator that referenced this pull request Jan 15, 2022
gund added a commit to orchestratora/orchestrator that referenced this pull request Jan 15, 2022
gund added a commit to orchestratora/orchestrator that referenced this pull request Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants