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

View flux editor in the CEO #4311

Merged
merged 5 commits into from
Aug 29, 2018
Merged

View flux editor in the CEO #4311

merged 5 commits into from
Aug 29, 2018

Conversation

AlirieGray
Copy link
Contributor

@AlirieGray AlirieGray commented Aug 28, 2018

Closes: https://github.com/influxdata/applications-team-issues/issues/3 and https://github.com/influxdata/applications-team-issues/issues/4

What was the problem?
Only InfluxQL queries could be created from the Cell Editor Overlay. Furthermore, there was some repetitiveness with shared/TimeMachine and flux/TimeMachine.

What was the solution?

  • Replace flux/TimeMachine with FluxQueryBuilder and use FluxQueryBuilder component in CEO
  • Move helper functions to a utils files that can be used for both FluxPage and CEO
  • Replace NotificationContext and FluxContext with passing props down
  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable
  • Tests pass

sources,
services,
meRole,
dashboard,
links: links.flux,
Copy link
Contributor

Choose a reason for hiding this comment

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

since these links are specific to flux, how do you feel about naming it something more specific like fluxLinks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

script={script}
status={status}
notify={notify}
context={this.getContext}
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you passing context in as both a context provider and a prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

@AlirieGray AlirieGray force-pushed the flux/flux-editor-ceo branch from 7bc86cd to a09a391 Compare August 29, 2018 18:01
@AlirieGray AlirieGray force-pushed the flux/flux-editor-ceo branch 2 times, most recently from 4b9b15c to 15cf1e8 Compare August 29, 2018 18:18
Copy link
Contributor

@ebb-tide ebb-tide left a comment

Choose a reason for hiding this comment

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

I'm on the fence about the decision to revert to passing props down instead of using contexts for notifications and flux context, but it's a matter of which one is slimmer to implement than the other- just noting that it's not a clear cut decision.

@@ -40,7 +40,7 @@ const SourceSelector: SFC<Props> = ({
sources={sources}
allowInfluxQL={true}
// TODO: when flux is added into CEO/DE, change to true
allowFlux={false}
allowFlux={process.env.NODE_ENV === 'development' ? true : false}
Copy link
Contributor

Choose a reason for hiding this comment

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

hah. cool :)

Copy link
Contributor

Choose a reason for hiding this comment

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

when will this be removed?

Copy link
Contributor Author

@AlirieGray AlirieGray Aug 29, 2018

Choose a reason for hiding this comment

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

Probably when the TimeMachine w/ Flux is added to the Data Explorer page. I believe @ischolten is working on that rn


const funcNode = (
<FuncNode
key={i}
Copy link
Contributor

Choose a reason for hiding this comment

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

since funcs have unique id's would you consider using those to uniquely key mapped items- instead of using their index? Using array index to generate keys is an anti pattern because react can't tell that things need to be updated if they change their order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

body: Body[]
service: Service
context: Context
Copy link
Contributor

Choose a reason for hiding this comment

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

if you are going to pass context down as props, could you name which context it is as well? maybe fluxContext.

onDeleteBody={onDeleteBody}
isLastBody={this.isLastBody(i)}
declarationsFromBody={this.declarationsFromBody}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this watts???? (he always re-orders things based on their length).

script: '',
}

this.debouncedASTResponse = _.debounce(script => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is cool!

@AlirieGray AlirieGray force-pushed the flux/flux-editor-ceo branch 3 times, most recently from 1d41999 to f671c9d Compare August 29, 2018 20:02
@AlirieGray AlirieGray force-pushed the flux/flux-editor-ceo branch from f671c9d to 3ee1597 Compare August 29, 2018 20:11
@AlirieGray AlirieGray merged commit b781d2e into master Aug 29, 2018
@AlirieGray AlirieGray deleted the flux/flux-editor-ceo branch August 29, 2018 20:33
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.

3 participants