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

Change spin/electronic type setting to toggle #297

Merged
merged 3 commits into from
Sep 22, 2022

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Sep 21, 2022

fixes #293

@superstar54
Copy link
Member

Could you please add a screenshot that quickly shows the UI before and after this PR?
I don't really agree with the idea that changing all choices to the toggle button. What if there are more choices, eg. ten options, will there be ten buttons?

@unkcpz
Copy link
Member Author

unkcpz commented Sep 21, 2022

Hi @superstar54, thanks for the quick reply. Here is the screenshot

Before:
image

After:
image

I don't really agree with the idea that changing all choices to the toggle button. What if there are more choices, eg. ten options, will there be ten buttons?

This is the feedback from Nicola actually. The widget for parameters setting need to be uniformed otherwise not very convenient for users. For this case explicitly, I think we don't have too many choices, the electronic type will influence how the occupation for pwscf, smearing for metal and fixed for insulator. The magnism will be none or collinear, all options are defined and can be find from the protocol https://github.com/aiidateam/aiida-quantumespresso/blob/main/src/aiida_quantumespresso/common/types.py

@superstar54
Copy link
Member

Thanks for the details. In this case, with only two options, I agree that the toggle button is better.

One more question. Is it easy to align left for the buttons: Metal and Off? It would look nicer if they were aligned.

@unkcpz
Copy link
Member Author

unkcpz commented Sep 21, 2022

@superstar54 that's a good point. It can but more or less increase the complexity of the code. Here it initially uses the {"description_width": "initial"} for the description of the widget as we usually set it for a simple widget. If we want more control, have to separate the description out as the ipw.Label as I did in the new commit.
But I think this is a fair change. Please check the screenshot below for what it looks like.
image

Copy link
Member

@superstar54 superstar54 left a comment

Choose a reason for hiding this comment

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

Great!

@unkcpz unkcpz merged commit ad8606f into master Sep 22, 2022
@unkcpz unkcpz deleted the fix/xx/elec-type-mag-using-toggle branch September 22, 2022 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants