-
Notifications
You must be signed in to change notification settings - Fork 15
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
Redesign summary view #1044
Redesign summary view #1044
Conversation
If anyone has an older container (with an older AiiDA version, say, 2.5ish), can you please verify that you still get the notification regarding the unavailability of the raw data download feature? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1044 +/- ##
==========================================
- Coverage 67.65% 67.58% -0.07%
==========================================
Files 117 118 +1
Lines 6498 6543 +45
==========================================
+ Hits 4396 4422 +26
- Misses 2102 2121 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
why dont you try with Also since the tab of picking the settings was Basic settings, you should keep the same instead of using Main settings |
No idea what you're referring to 😅
This was a specific request by Giovanni. See issue. |
You can make an HTML object and create a table and to display it , it has better visuals than the ipywidget.HTML.
Ok , then we should keep it consistent in the configuration step , and name it main settings as well (instead of basic) |
e5705a1
to
69de3fa
Compare
@edan-bainglass I can confirm you that the notification for aiida 2.5 works |
Thanks @AndresOrtegaGuerrero 🙏 |
@giovannipizzi @cpignedoli alternative... Both Also, "Main settings" is inconsistent with "Basic settings". Best we uniform this. Do you agree, and if so, preference? |
69de3fa
to
83bc48a
Compare
Thanks! Both are good, table is OK as the dark color is not too dark/prominent. But as the list might get longer, maybe the list is OK. I agree with uniforming basic/main,maybe "main" is better but both are are ok. |
Okay then. Leaving For reference |
Adding more info requested in #1042 and |
59ed35f
to
a8399d0
Compare
@AndresOrtegaGuerrero could you please review once more? 🙏 |
@edan-bainglass Thank you! In my test everything works. I did notice an issue I mentioned in #1045, where we sometimes fail to indicate whether the full workflow completed successfully. However, I’m thinking this might not be the right place to address it—what do you think? |
Just dont forget to solve the conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you @edan-bainglass
@edan-bainglass It just crossed my mind now, should we report the properties computed ? |
Indeed, unrelated. Thanks for opening the issue. |
I suppose so? Doesn't hurt. I'll add it 👍 |
@AndresOrtegaGuerrero not sure where and how to best represent the computed properties. For now, I'll merge without it. Let's discuss and PR this later. |
a8399d0
to
c6e1725
Compare
This PR is one in several that will address #1042
Here, I introduce a bullet list alternative format for the summary report (no actual bullets). The format is defined in
qeapp.yml
assummary_format: list
and can be overridden by the user in a config file (see #1007 for info regarding file).summary_format
takeslist
ortable
.I also adjust the style such that the data download widget is to the right of the summary instead of below. This is true for either selected format.
Lastly, I add "Workflow properties" and "Structure properties" sections to the summary.