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 oceanBottomPressure eov #407

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

JessyBarrette
Copy link
Contributor

Add ocean Bottom pressure eov to cde as requested by ONC.

See goose eov for more details.

I'm not sure if there's more needed. I'm also not sure if the sea-level.svg icon is the appropriate one for it. Though those icons do not seem to be stored within this repo.

@JessyBarrette
Copy link
Contributor Author

this is related to #406

n-a-t-e
n-a-t-e previously approved these changes Apr 4, 2024
@n-a-t-e n-a-t-e dismissed their stale review April 4, 2024 16:38

need to look over something

@n-a-t-e
Copy link
Member

n-a-t-e commented Apr 4, 2024

I think there are more spots the EOV is referenced in the codebase. If you do a search through the entire codebase for an existing EOV, eg dissolvedOrganicCarbon, you should be able to see where else this new EOV needs to be added. Eg, translation.json

@JessyBarrette
Copy link
Contributor Author

I think this is integrating all the different places where oceanBottomPressure should be added. I'll review if the deployment is complaining or not

@n-a-t-e
Copy link
Member

n-a-t-e commented May 21, 2024

I think this is integrating all the different places where oceanBottomPressure should be added. I'll review if the deployment is complaining or not

By deployment do you mean see if it works after deployed to the live site?

It would be great to test this PR in a development environment

@JessyBarrette
Copy link
Contributor Author

oh sorry I meant locally

@JessyBarrette
Copy link
Contributor Author

This PR brought in Ocean Bottom Pressure EOV locally
image

@JessyBarrette
Copy link
Contributor Author

@n-a-t-e I successfully deployed this latest PR locally and did see the new EOV available via the UI. I think we're good to go. Just maybe approuve it and then we can see how to update the production deployment of CDE with those latest changes.

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