Closed e0 closed 2 years ago
Here's a comment before the review (posting it now so as not to forget): if the gemRepository
folder is required, perhaps it should be added to the repo (with a blank .keep
inside) and simply ignore anything else inside that folder.
Here's a comment before the review (posting it now so as not to forget): if the
gemRepository
folder is required, perhaps it should be added to the repo (with a blank.keep
inside) and simply ignore anything else inside that folder.
I think it is customary to not include any type of data output or build folders in git repos. I do think we could automate this process though so that people would not have to manually create any of the data output folders.
It would have been nice to have had them in a separate commit or perhaps even a separate branch.
I had that thought as well, but didn't feel strong enough about it to type it out 😄
One thing which made the reviewing harder is that there seem to be a lot of changes introduced related to linting/formatting, 11 files has changed in total in this PR. It would have been nice to have had them in a separate commit or perhaps even a separate branch.
This is very valid criticism, especially considering that I didn't consult anyone about this prior to checking in the changes. Sorry! Would you prefer if I reverted the pure formatting lines?
This looks like a solid MVP. I have some comments and improvement suggestions:
- Any ideas how to deal with the cases where bubbles overlap?
I had a few ideas but none that are straightforward to implement. For example if there are two or more version too close together, we could consolidate them into a single node. That would need to be elaborated more in regards to how we deal with the popover and potential branching issues.
Another thing we could do is to have different major versions on different lines. Personally I find it weird that that version 8.0.0
appears before some 7.*
versions. I get that it's common practice but it's confusing to see it visualized on the same line.
I didn't mention these when bringing them up here because I don't like either of the suggestions since they introduce more problems. I think it's worth pursuing other ideas and suggestions here.
- It can be tricky when branches go behind bubbles. One idea could be to color branches, or to not overlap them.
I think this is a nice to haves and also a bit opinionated. I don't think I have time to work on it this week. If anyone wants to, please go ahead.
I think the chart would look cleaner if the months would not be in bold, but I'm also onboard with skipping the months completely.
The model names have to end with
-GEM
, even though it appears like a "waste of space". With the integration of more datasets for when building this tree, the SVG will have to be horizontally scrollable anyway.
Since this seems like a hard requirement to add the -GEM
, I have added a commit here: https://github.com/MetabolicAtlas/data-generation/pull/27/commits/62279b672e27ab847db03cffe1815a71f0753ff4. Coincidentally, d3
auto optimized the time axis so that the months are skipped. :D
Since this has two approvals I think we can merge it for now. Sorry again for the prettier changes.
This PR is related to https://github.com/MetabolicAtlas/MetabolicAtlas/issues/949.
Type of change
List of changes made
import
instead ofrequire
). This is needed because modern versions ofd3
need ES Modules and do not work withrequire
.Testing
gemRepository
folder if necessary:mkdir gemRepository
.yarn
.yarn start ../data-files
.gemRepository
folder.