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

add original flow input #137

Merged
merged 10 commits into from
Feb 7, 2021
Merged

add original flow input #137

merged 10 commits into from
Feb 7, 2021

Conversation

yehiyam
Copy link
Contributor

@yehiyam yehiyam commented Feb 4, 2021

This change is Reviewable

@yehiyam yehiyam requested a review from reggev February 4, 2021 14:59
@yehiyam
Copy link
Contributor Author

yehiyam commented Feb 4, 2021

/deploy dev1

@hkube-ci hkube-ci temporarily deployed to dev1 February 4, 2021 15:10 Inactive
@yehiyam
Copy link
Contributor Author

yehiyam commented Feb 4, 2021

/deploy dev1

@hkube-ci hkube-ci temporarily deployed to dev1 February 4, 2021 15:57 Inactive
Copy link
Contributor

@reggev reggev left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions (waiting on @yehiyam)


src/components/common/Json/JsonSwitch/JsonSwitch.react.js, line 59 at r3 (raw file):

  // eslint-disable-next-line
  options: PropTypes.object,
  // eslint-disable-next-line

If you don't want to detail the props you can just wrap the whole block with:
/* eslint-disable react/forbid-prop-types /
// ignored props
/
eslint-enable react/forbid-prop-types */
If you can, try to detail as mush as you can, it helps catching errors.


src/components/common/Json/JsonSwitch/JsonSwitch.react.js, line 60 at r3 (raw file):

  options: PropTypes.object,
  // eslint-disable-next-line
  jobId: PropTypes.string,

can you provide some default value or pull it to null if nothing is provided?
if it's required then mark it as required


src/components/common/Json/JsonTable/JsonTable.react.js, line 47 at r3 (raw file):

        <>
          <Button onClick={handleDownload}>Download</Button>
          <a

Iv'e implemented a file called DownloadLink.js on a branch 'bugfix-datasources', that does this whole process of creating a link and clicking it whenever the href prop changes
It'll be better if you use that, I'm trying to get rid of all the uses of the socket's URL (or any other URL constructing method) inside the components...


src/components/common/Json/JsonTable/JsonTable.react.js, line 93 at r3 (raw file):

  isMargin: PropTypes.bool,
  key: PropTypes.oneOf([PropTypes.string, PropTypes.number]),
  jobId: PropTypes.string,

Is it a required property?


src/hooks/usePipeline.js, line 28 at r3 (raw file):

    [updateStored]
  );
  const rerunRawPipeline = async (pipeline, jobId) => {

consider wrapping this in a useCallback

Copy link
Contributor Author

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @reggev)


src/components/common/Json/JsonSwitch/JsonSwitch.react.js, line 59 at r3 (raw file):

Previously, reggev wrote…

If you don't want to detail the props you can just wrap the whole block with:
/* eslint-disable react/forbid-prop-types /
// ignored props
/
eslint-enable react/forbid-prop-types */
If you can, try to detail as mush as you can, it helps catching errors.

added more detail


src/components/common/Json/JsonSwitch/JsonSwitch.react.js, line 60 at r3 (raw file):

Previously, reggev wrote…

can you provide some default value or pull it to null if nothing is provided?
if it's required then mark it as required

made it required


src/components/common/Json/JsonTable/JsonTable.react.js, line 47 at r3 (raw file):

Previously, reggev wrote…

Iv'e implemented a file called DownloadLink.js on a branch 'bugfix-datasources', that does this whole process of creating a link and clicking it whenever the href prop changes
It'll be better if you use that, I'm trying to get rid of all the uses of the socket's URL (or any other URL constructing method) inside the components...

Done.


src/components/common/Json/JsonTable/JsonTable.react.js, line 93 at r3 (raw file):

Previously, reggev wrote…

Is it a required property?

Done.


src/hooks/usePipeline.js, line 28 at r3 (raw file):

Previously, reggev wrote…

consider wrapping this in a useCallback

not sure how can I use useCallback here

@yehiyam
Copy link
Contributor Author

yehiyam commented Feb 7, 2021

/deploy dev1

@hkube-ci hkube-ci temporarily deployed to dev1 February 7, 2021 13:48 Inactive
Copy link
Contributor

@reggev reggev left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @yehiyam)


src/hooks/usePipeline.js, line 28 at r3 (raw file):

Previously, yehiyam wrote…

not sure how can I use useCallback here

I added for you

@reggev
Copy link
Contributor

reggev commented Feb 7, 2021

/deploy dev1

@hkube-ci hkube-ci temporarily deployed to dev1 February 7, 2021 14:53 Inactive
@reggev
Copy link
Contributor

reggev commented Feb 7, 2021

/deploy dev1

@hkube-ci hkube-ci temporarily deployed to dev1 February 7, 2021 15:09 Inactive
Copy link
Contributor

@reggev reggev left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @yehiyam)

@reggev reggev merged commit 85983b1 into master Feb 7, 2021
@reggev reggev deleted the get-flow-input branch February 7, 2021 15:38
hkube-ci pushed a commit that referenced this pull request Feb 7, 2021
add original flow input .... bump version [skip ci]
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