Skip to content

Conversation

shammamah-zz
Copy link
Contributor

@shammamah-zz shammamah-zz commented Apr 10, 2019

Description of changes

Previously, the Speck component was trying to calculate the radius for an atom even if no atoms were present in the system. This led to an exception and a failure to render.

Now, the function loadStructure simply returns if there are no atoms supplied in the system and consequently avoids this error.

I also fixed the CHANGELOG to have nicer formatting and added the fix from #311 to the notes for the 0.0.9 release.

Before merging

@shammamah-zz shammamah-zz requested a review from mkcor April 10, 2019 19:15
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-312 April 10, 2019 19:15 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-312 April 10, 2019 19:16 Inactive
This reverts commit b90c0f9.
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-312 April 10, 2019 19:20 Inactive
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-312 April 10, 2019 19:37 Inactive
@shammamah-zz
Copy link
Contributor Author

The previous deployments were failing due to #313. I've added a temporary fix by locking the Dash version to 0.40.0, which uses the last version of dash that accepts React.PropTypes.

CHANGELOG.md Outdated
figure.
* Fixed issue with Speck not rendering unless it is attached to a callback.
* Fixed issue with Speck trying to calculate a system with no atoms,
which led to an error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest removing this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkcor This is an issue that was fixed, though -- do you have an idea of how it could be worded better? (Or does this belong in another section?)

Copy link
Contributor

Choose a reason for hiding this comment

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

My rewording suggestion corresponds to making the whole sentence fit in one line, which is "Prevent Speck from trying to calculate a system with no atom." It says it all. We fixed an issue (when there was an issue, there was an error, but it's trivial so there's no need to specify it; it's not surprising that trying to calculate an empty system would lead to an error; it's not interesting to write out "there used to be an error" and the source code is clear and commented...).

"Prevent Speck from trying to calculate a system with no atom" makes it clear that previously Speck was trying to calculate such a system and clearly that was an issue!!

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 clarification :) Fixed in 8a1f343!

@mkcor mkcor mentioned this pull request Apr 11, 2019
6 tasks
Small wording/spelling fixes.

Co-Authored-By: shammamah <shammamah.hossain@gmail.com>
@shammamah-zz shammamah-zz temporarily deployed to dash-bio-test-pr-312 April 11, 2019 13:37 Inactive
@shammamah-zz shammamah-zz merged commit 5839ea5 into master Apr 11, 2019
@shammamah-zz shammamah-zz deleted the speck-system-fix branch April 11, 2019 14:05
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.

2 participants