-
Notifications
You must be signed in to change notification settings - Fork 903
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
refactor(core): Convert most triggers from angular to react #6970
refactor(core): Convert most triggers from angular to react #6970
Conversation
@spinnakerbot add-label angular2react |
@christopherthielen Pretty big refactor on the individual triggers. Cron is going to require a little more time due to the fact it made use of an external angular library |
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.
Great work, this is awesome!
One thing I'd like to avoid is using component inheritance when possible. React docs recommend against this and suggest using composition instead https://reactjs.org/docs/composition-vs-inheritance.html
For example, in BaseBuildTriggerTemplate, instead of using an abstract method getBuildTriggerType()
, you can add a triggerType
prop to the component and then pass that prop in from the more specific component, e.g:
const JenkinsTriggerTemplate = () => <BaseBuildTriggerTemplate triggerType={BuildServiceType.Jenkins} />
(side note: then the component could be renamed to BuildTriggerTemplate
as well)
In the case of something more complex such as BaseBuildTrigger
which is loading data, etc in the parent class, you can use a render
prop to pass data to a child in an unopinionated way.
That noted, I'm OK with merging this as-is and refactoring later, if it makes sense to invest in that.
app/scripts/modules/core/src/pipeline/config/triggers/BaseTrigger.tsx
Outdated
Show resolved
Hide resolved
app/scripts/modules/core/src/pipeline/config/triggers/baseBuild/BaseBuildTrigger.tsx
Outdated
Show resolved
Hide resolved
1b5d927
to
d0fd709
Compare
@christopherthielen I've made the changes to use composition as you suggested. If everything looks okay I'll merge. Thanks! |
d0fd709
to
8f4cca9
Compare
- all triggers but CRON have been converted - triggers which have already been converted to react have been unified spinnaker/spinnaker#4295
8f4cca9
to
c8590ff
Compare
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.
Love it, thanks for making those changes!!
Regression introduced in spinnaker#6970 while converting triggers to React
Regression introduced in #6970 while converting triggers to React
spinnaker/spinnaker#4295