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

fix(flow): some refactor for flows #2164

Merged
merged 18 commits into from
Aug 2, 2019
Merged

fix(flow): some refactor for flows #2164

merged 18 commits into from
Aug 2, 2019

Conversation

allardy
Copy link
Member

@allardy allardy commented Jul 25, 2019

Added basis for new node types. Also fixes some issues left of my pr of custom URL that i've encountered..

@allardy allardy requested a review from slvnperron July 29, 2019 19:44
@@ -119,7 +119,7 @@ class LoggerView extends Component {
<ul className={styles.events}>{logs}</ul>
</div>
{canLoadMore && (
<div href="#" className={styles['logs-panel-footer']} onClick={this.loadMore}>
<div className={styles['logs-panel-footer']} onClick={this.loadMore}>
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this file entirely

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'll remove it in a subsequent PR, I removed the # because it caused issue with baseurl, but it was unnecessary

Copy link
Member

@EFF EFF left a comment

Choose a reason for hiding this comment

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

can't wait to try this out !

src/bp/common/typings.ts Outdated Show resolved Hide resolved
links: NodeLinkView[]
}

export type NodeLinkView = {
Copy link
Member

Choose a reason for hiding this comment

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

In a traditional graph, that would be called an edge

Copy link
Member Author

Choose a reason for hiding this comment

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

Existing typings that i've moved, I didn't thought much about renaming them

import _ from 'lodash'
import Mustache from 'mustache'
import React, { Component } from 'react'
import { OverlayTrigger, Popover, Well } from 'react-bootstrap'
Copy link
Member

Choose a reason for hiding this comment

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

can't we use blueprint components instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's existing code that i've copy-pasted, since we're gonna change all that soon I didn't bother to convert it. It keeps the same look as old nodes (since they are temporary)

Copy link
Member

Choose a reason for hiding this comment

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

ok cool but let's add a TODO because we're going to migrate the styling to blueprint before we get rid of the nodes

<a href="#" onClick={this.markAllAsRead}>
Mark all as read
</a>
<a onClick={this.markAllAsRead}>Mark all as read</a>
Copy link
Member

Choose a reason for hiding this comment

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

is the notification hub even working ? I mean do we even send notification somewhere ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, still used in HITL for now

showActionModalForm: false
}

itemToOptions(item) {
Copy link
Member

Choose a reason for hiding this comment

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

that's some parsing magic, I'm totally out of context I know but do you think we could eventually get rid of this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not the way nodes are done in flow v1. I've copied the code from ActionModalForm but made it into a smaller component for that purpose, but all this is throwable

import _ from 'lodash'
import Mustache from 'mustache'
import React, { Component } from 'react'
import { OverlayTrigger, Popover, Well } from 'react-bootstrap'
Copy link
Member

Choose a reason for hiding this comment

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

ok cool but let's add a TODO because we're going to migrate the styling to blueprint before we get rid of the nodes

@allardy allardy merged commit a1a985e into minor/12-1-0 Aug 2, 2019
@slvnperron slvnperron deleted the ya-flow-newnodes branch August 18, 2019 15:40
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