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

Redesign header with logo, toggled info sections, etc. #751

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Jun 24, 2024

Based on #772

This PR replaces refactors the main notebook as follows:

  • Much of the notebook is moved out to an AppWrapper component
  • The AppWrapper component holds the banner, main, and footer components
  • The banner component (used to be the welcome message) is redesigned as follows:
    • App logo (same as ReadTheDocs logo) and subtitle
    • Toggle buttons triggering an info box optionally showing one of the following:
      • Guide section - the old welcome page plus links to online docs (see Implement path for in-app guides #763 for future direction)
      • About section - general information about the app and acknowledgements
  • The actual app is injected from the notebook into the main component
  • The footer component showing copyrights and version

Use of CSS

The notebook now also loads two CSS stylesheets:

  1. style (style.css) - this was already used in the welcome page
  2. custom (custom.css, compiled from custom.scss) - this is used for any custom styling we wish to add anywhere in the app

The general choice to use CSS is three-fold:

  1. Ease-of-use - we're already using a limited version via ipywidgets styling API
  2. Separation of responsibilities - all styling moves to stylesheets
  3. Flexibility - ipywidgets only exposes a limited set of CSS
    • For example, the Checkbox widget is comprised of multiple HTML elements
    • ipywidgets does not provide a path for styling each
    • CSS has no limitations here.

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.

Thanks, @edan-bainglass for the new header design. Very nice!
Few suggestions.

  • Better to move the four-steps instruction in the About section to the Guide section. Instead, you can add the the more general info, e.g., what the capability of the app (Bands, PDOS, XPS, IR/Raman, etc).
  • Regarding the in-app guide, we can discuss on detail implementaion . For this PR, could you hide the show in-app guide checkbox and related.
  • The custom CSS is kind of advanced topic, and would be very useful if you add some docs on it (e.g., the development guide).

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.74%. Comparing base (e492404) to head (03d5f11).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   68.16%   68.74%   +0.57%     
==========================================
  Files          48       49       +1     
  Lines        4159     4236      +77     
==========================================
+ Hits         2835     2912      +77     
  Misses       1324     1324              
Flag Coverage Δ
python-3.11 68.74% <100.00%> (+0.57%) ⬆️
python-3.9 68.77% <100.00%> (+0.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielhollas
Copy link
Contributor

@edan-bainglass LMK once this is ready and I'll have a closer look (just a note that I am at the conference next week, so will get it no sooner than July 14th.

@edan-bainglass
Copy link
Member Author

@danielhollas @superstar54 ready for review

@edan-bainglass
Copy link
Member Author

Thanks, @edan-bainglass for the new header design. Very nice! Few suggestions.

  • Better to move the four-steps instruction in the About section to the Guide section. Instead, you can add the the more general info, e.g., what the capability of the app (Bands, PDOS, XPS, IR/Raman, etc).
  • Regarding the in-app guide, we can discuss on detail implementaion . For this PR, could you hide the show in-app guide checkbox and related.
  • The custom CSS is kind of advanced topic, and would be very useful if you add some docs on it (e.g., the development guide).

@superstar54 your comments have all been addressed. Note that the last one regarding documenting CSS use - the PR introducing the CSS loader in AWB (aiidalab/aiidalab-widgets-base#624) adds docs there regarding CSS.

Good to merge?

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.

Hi @edan-bainglass , thanks for the work! I added two mirror change requests. Ready to go after you fix them.

src/aiidalab_qe/app/static/templates/about.jinja Outdated Show resolved Hide resolved
"metadata": {},
"outputs": [],
"source": [
"controller.enable_toggles()"
Copy link
Member

Choose a reason for hiding this comment

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

Why only enable the toggler at the last? Is it possible to show the Guide and About when loading the main app?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately no 😞 Loading the app is an event. Clicking on guide or about will add that event behind the loading event, i.e., it will only trigger after the app is done loading. This is bad UX, so I enable the buttons only once the app finishes loading.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this means that the user can not do anything when loading the main app. I hope we can find some solution in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make the app load faster 😅 Working on it

Co-authored-by: Xing Wang <[email protected]>
@edan-bainglass edan-bainglass merged commit 2c22abb into main Sep 17, 2024
12 checks passed
@edan-bainglass edan-bainglass deleted the header-redesign branch September 17, 2024 09:15
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.

4 participants