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

feat: update new trigger modal according to design #1786

Merged
merged 25 commits into from
Mar 1, 2020

Conversation

liweitian
Copy link
Contributor

@liweitian liweitian commented Dec 18, 2019

Description

Update according to new trigger design. Will replace the inline editor after it is done.

Task Item

Closes #1641

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

1111

Syntax Highlight & Completion
image

@github-actions
Copy link

Coverage Status

Coverage increased (+0.02%) to 40.27% when pulling cc79ecb6e6aaab2e52136f7b76884b9bcb0f5d72 on liweitian/newTriggerModal into 4ea1b2a on master.

@liweitian liweitian changed the title update new trigger modal according to design feat: update new trigger modal according to design Dec 18, 2019
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

Some small changes.

@liweitian liweitian force-pushed the liweitian/newTriggerModal branch from 3db13e6 to 5528ba3 Compare December 19, 2019 08:05
Copy link
Contributor

@a-b-r-o-w-n a-b-r-o-w-n left a comment

Choose a reason for hiding this comment

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

The only other thing here is this odd spacing issue:

image

@cwhitten
Copy link
Member

@liweitian I'd like to hold on this until we have the inline monaco editor to provide syntax highlighting and validation.

Copy link
Contributor

@yeze322 yeze322 left a comment

Choose a reason for hiding this comment

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

Need some changes on code style

@boydc2014
Copy link
Contributor

Inline lu lsp is in, please merge master and resolve all the comments

@liweitian liweitian force-pushed the liweitian/newTriggerModal branch from 7ea144b to ebbc01a Compare February 26, 2020 05:21
@liweitian liweitian force-pushed the liweitian/newTriggerModal branch from 94154c7 to ba98fe8 Compare February 27, 2020 02:39
Copy link
Contributor

@yeze322 yeze322 left a comment

Choose a reason for hiding this comment

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

code style & mock module.

@yeze322 yeze322 self-requested a review February 28, 2020 14:57
@yeze322
Copy link
Contributor

yeze322 commented Feb 28, 2020

All my comments resolved! great 👍

@cwhitten cwhitten dismissed stale reviews from yeze322 and a-b-r-o-w-n February 28, 2020 18:12

stale

Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

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

Something I'm seeing on this diff is that I can no longer toggle into "Edit" mode in the all-up LU page. We need to preserve the capability for users to see the "raw" lu data.

onChange: (template?: string) => void;
}

export class LuEditorWidget extends React.Component<LuEditorWidgetProps> {
Copy link
Member

Choose a reason for hiding this comment

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

Let's convert this to a function component and use hooks instead of class methods.

I believe the getDerivedStateFromProps implementation is causing some unexpected behavior in editing the form. For example, data is getting overwritten at times, and I'm unable to add newlines to the bottom of an intent definition like I can in the inline LG editor. The behavior should be almost identical to the inline LG editor. See LgEditorWidget.tsx for its implementation. It should be used as a strong reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @yeze322 said,

This component has several problems. The usage of useEffect hook is anti-pattern meanwhile the data flow is not well designed (as an example, the onChange handler submits data to both local states and external API).

The props and local states are mixed together, makes your component hard to maintain.

You really need to rethink what's your scenario is and carefully define the shape of your input props to make sure you can fully leverage change detection mechanism.
Though state management of Shell has historical problems, you can try the Class Component with lifecycle methods before Leilei's fix on Shell states.
(I was also trapped in this problem before)

There is a bug about luEditor value in luEditorWidget: state localVale and props luIntent.body are mixed. So I try to use class component to fix it.
I fixed the bug: unable to add newlines to the bottom.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. I cut this: #2138 to follow-up.

Copy link
Contributor

@yeze322 yeze322 Mar 2, 2020

Choose a reason for hiding this comment

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

Under current data flow, getDerivedStateFromProps can match weitian's scenario though it's still not a good pattern. Then we can optimize this pattern by following best practice.

A suggested pattern of this scenario is to use a fully uncontrolled component with a key (according to official doc). A key here might be the data version of current local state, requires more shell or indexer work.

@cwhitten I will follow up in the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants