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

Fix StructureMoleculeComponent legend if same element has multiple oxi states #339

Merged
merged 3 commits into from
May 30, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented May 18, 2023

Closes #338.

This is a WIP to start gathering feedback. As suggested by @yang-ruoxi, this first step splits every oxi state into its own legend label without affecting the site colors. I.e. different oxi states still have the same color in the structure.

Suggestion: Make negative oxi states slightly darker, positive ones slightly brighter and combine different oxi states for the same element into 1 wider label.

This PR requires upstream changes in pymatgen: materialsproject/pymatgen#2998 i.e. we can't put this into prod until pymatgen cuts a new release.

@mkhorton If you have a minute, could you let me know if there's any problem with making StructureMoleculeComponent.get_scene_and_legend() non-static? See e418cae.

Here's what this PR looks like for the problematic structure reported by @ardunn on matsci.org.

Screenshot 2023-05-17 at 18 03 24

@janosh janosh added ui Styles, CSS, interactivity controls fix Bug fix PRs labels May 18, 2023
@yang-ruoxi
Copy link
Member

thanks @janosh !! It looks nice. I can see downstream people ask for color distinction for different oxi states, but as an incremental fix for the labels I think this is sufficient.

@janosh
Copy link
Member Author

janosh commented May 22, 2023

Sure happy to merge as is. We just need to await the next pymatgen release. Also, would be great if @mkhorton or @tschaume or whoever can invite me to the Percy project to green-light the visual changes causing CI to fail.

percy.io
Screenshot 2023-05-22 at 11 00 25

@tschaume tschaume self-assigned this May 22, 2023
@janosh janosh merged commit bc196ca into main May 30, 2023
@janosh janosh deleted the fix-struct-viewer-oxi-state-in-legend branch May 30, 2023 17:31
@janosh
Copy link
Member Author

janosh commented May 30, 2023

New pymatgen release to go along with this merge planned for later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix PRs ui Styles, CSS, interactivity controls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Structure viewer bundles elements with different oxidation states into same legend label
3 participants