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.

Sign up for GitHub

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

Jump to bottom

Retrieve split up JSON files #62

Open
wants to merge 21 commits into
base: devel
Choose a base branch
from
Open

Retrieve split up JSON files #62

wants to merge 21 commits into from

Conversation

gareth-j
Copy link
Contributor

@gareth-j gareth-j commented Sep 5, 2023 •
edited
Loading

  • Describe the changes implemented in this PR (Bug fix, feature, docs update, ...)
    This implements the dynamic retrieval of JSON files which hold each source (species, site, instrument, inlet...)

NOTE: This currently just uses a sample of the DECC data (CO2, CH4 and CO) and uses a temporary data repository (https://github.com/openghg/temp_data_dashboard). These will change before this PR is merged.

  • Please check if the PR fulfils these requirements
  • Closes Implement multi-file data retrieval #45
  • [Tests added and passed] if fixing a bug or adding a new feature
  • Documentation and tutorials updated/added
  • Added an entry in the latest CHANGELOG.md file if fixing a bug or adding a new feature
  • ESLINT passed

TODO

  • Fix this.props.siteStructure is undefined on clicking on on Background
  • Add tests
  • Add in config file for setup of remote data repository instead of hardcoding URL

@gareth-j gareth-j added the enhancement New feature or request label Sep 5, 2023
@gareth-j gareth-j self-assigned this Sep 5, 2023
@gareth-j gareth-j requested a review from cypressf September 7, 2023 09:36
@gareth-j gareth-j marked this pull request as ready for review September 7, 2023 09:36
@gareth-j gareth-j marked this pull request as draft September 7, 2023 09:36
@gareth-j
Copy link
Contributor Author

gareth-j commented Sep 7, 2023

Most of the work is done. I've just encountered a nasty bug when clicking on the Background button in the sidebar. I'll take a look at this next week as I'm moving onto my other project today. I think the rest of the code is ready for review though.

@gareth-j gareth-j marked this pull request as ready for review September 7, 2023 09:37
SutarPrasad
SutarPrasad reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@SutarPrasad SutarPrasad left a comment

Choose a reason for hiding this comment

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

Looks Good. One small suggestion was added as a comment.

src/components/MultiSiteLineChart/MultiSiteLineChart.js
</Button>

if (plotData.length === 0) {
return <div>Retrieving data...</div>;
Copy link
Member

Choose a reason for hiding this comment

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

An icon from here can be helpful:
https://mui.com/material-ui/material-icons/?query=loading

@cypressf
Copy link
Collaborator

I think chunking data makes sense if you've exhausted the potential for data compression, alternate data file formats, and reducing needless precision. Have you determined that even with those three approaches the total size is still not under 1MB? If so, ship it! If not, I'd encourage you to investigate those to see if you could keep the code simple and still load all the data at once.

@gareth-j gareth-j changed the base branch from main to devel September 22, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SutarPrasad SutarPrasad SutarPrasad left review comments

@cypressf cypressf cypressf left review comments

Assignees

@gareth-j gareth-j

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement multi-file data retrieval
3 participants
@gareth-j @cypressf @SutarPrasad