Skip to content

Conversation

rpkyle
Copy link
Contributor

@rpkyle rpkyle commented Jun 27, 2019

About

  • I am updating the R package which provides dash-bio support in Dash for R.

Description of changes

Before merging

@rpkyle rpkyle requested a review from shammamah-zz June 27, 2019 17:05
@rpkyle rpkyle self-assigned this Jun 27, 2019
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 June 27, 2019 17:06 Inactive
Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

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

💃 But with some small comments!

DESCRIPTION Outdated
Depends: R (>= 3.0.2), manhattanly
Imports: dash
Suggests:
Suggests:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this whitespace be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can fix that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 26f0401.

- color (character; required): the color of the block.
- label (character; required): the labels of the block.
- id (character; required): the id of the block, where it will recieve
data from the specified "track" id.s. The overall layout of the Circos graph, provided
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to have everything after/including "The overall layout..." before this list -- it kind of looks like it's part of the last element in the list, but it isn't. The extraneous s. should be covered by my changes in plotly/dash#782, but those haven't been released yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a tricky one (but a very valid point). It's because the generator detects that layout has PropTypes.exact, and so it handles the nested typing. Because it's a typing argument, it's at the head of the string (as are all types).

I'm not entirely sure how we would break the typing string in half, and insert the The overall layout of the Circos graph, provided as a list of dictionaries. after ..., 'id'. but I can look into it for the next release. I'll open an issue in the dash repo so I don't lose track of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback; noted in plotly/dash#799.

.Rbuildignore Outdated
index.py
config.py
usage.py
setup.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a regression... I remember reviewing this before, but I can't break the commits down because it looks like they were rebased into 28c375b.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noting this. Fixed in d7bd0d4.

.Rbuildignore Outdated
.pylintrc
test/
# CRAN has weird LICENSE requirements
LICENSE.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I remember reviewing this and @shammamah had fixed it accordingly.

Copy link
Contributor Author

@rpkyle rpkyle Jul 2, 2019

Choose a reason for hiding this comment

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

Fixed in d7bd0d4.

@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 July 2, 2019 19:04 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 July 2, 2019 19:07 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 July 2, 2019 19:17 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 July 2, 2019 19:20 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-382 July 2, 2019 19:22 Inactive
@rpkyle rpkyle merged commit 324c49d into master Jul 2, 2019
@rpkyle rpkyle deleted the update-for-dash-0.1.0 branch July 2, 2019 19:38
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