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

Update chemenv documetation and suggestions #771

Merged
merged 10 commits into from
May 6, 2023

Conversation

JaGeo
Copy link
Member

@JaGeo JaGeo commented Apr 24, 2023

@munrojm , I had a quick look.

I think, it would be useful to be able to search for elements and species as well (for example, if you would like to search for new Li ion conductors). In addition, I very much like chemenv_symbol as well as this is directly linked to the ChemEnv implementation.
Could we also make it more clear which kind of options for chemenv_... exist? I added a bit in the documentation but as I defined the Literals maybe more options exist.

Let me know what you think.

I also found some tiny typos in the documentation.

@munrojm
Copy link
Member

munrojm commented Apr 24, 2023

Absolutely, very happy to add that. I'll push the changes here and we can iterate.

@munrojm
Copy link
Member

munrojm commented Apr 25, 2023

@JaGeo, I have updated the client to be commensurate with the changes in materialsproject/emmet#703.

Once deployed we can merge this if you are okay with the changes.

@JaGeo JaGeo changed the title Update documetation and suggestions Update chemenv documetation and suggestions Apr 26, 2023
@JaGeo
Copy link
Member Author

JaGeo commented Apr 26, 2023

@munrojm , thank you. I will then test it after the deployment has happened. I now just made a tiny change on the documentation.

@JaGeo
Copy link
Member Author

JaGeo commented May 6, 2023

@munrojm let me know if I can do anything more. In any case, thank you for all the work!

@munrojm
Copy link
Member

munrojm commented May 6, 2023

No worries! We just deployed, so I wanted to add tests and get everything to pass before pinging you to take a look before we merge. I should also point out that we have a preliminary table on the details pages showing some of the data. Here is an example. Any changes or additions you would like to make are very welcome.

@munrojm
Copy link
Member

munrojm commented May 6, 2023

@JaGeo Actually, I am just realizing that this PR won't have tests pass because of API key issues. Everything passes when running locally, so I would just take a look and test the client now with the deployed API. If you are okay with everything, we can merge. Happy to iterate too.

@JaGeo
Copy link
Member Author

JaGeo commented May 6, 2023

@munrojm To me, it looks good! I think you can now query everything important for the environments. Thanks for this!

@munrojm
Copy link
Member

munrojm commented May 6, 2023

@JaGeo Sounds good!

@munrojm munrojm added the release:patch Patch release label May 6, 2023
@munrojm munrojm merged commit aa93193 into materialsproject:main May 6, 2023
2 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:patch Patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants