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

Implement path for in-app guides #763

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

edan-bainglass
Copy link
Member

@edan-bainglass edan-bainglass commented Jul 3, 2024

This PR adds a mechanism for adding in-app guides meant as example use-cases for users.

In-app guide widgets can be added anywhere as follows:

from aiidalab_qe.common.infobox import InAppGuide

example = InAppGuide(
    children=[
        ipw.HTML("An example in-app guide."),
    ],
    guide_id="example",
)

The "child" of the widget can be any other widget supported by ipywidgets. This allows developers to customize the content of the guide as they wish. The example CSS class provided via guide_id marks the widget as part of the "example" guide. This is used for toggling between guides.

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 67.18%. Comparing base (b0cfdbd) to head (5ebeedc).

Files with missing lines Patch % Lines
src/aiidalab_qe/app/wrapper.py 68.75% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #763      +/-   ##
==========================================
+ Coverage   67.15%   67.18%   +0.02%     
==========================================
  Files          51       51              
  Lines        4707     4720      +13     
==========================================
+ Hits         3161     3171      +10     
- Misses       1546     1549       +3     
Flag Coverage Δ
python-3.11 67.18% <75.00%> (+0.02%) ⬆️
python-3.9 67.21% <75.00%> (+0.02%) ⬆️

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.

@superstar54
Copy link
Member

Hi @edan-bainglass , is this PR ready for review? I like the design, and I am looking forward to having it in the APP.

@edan-bainglass
Copy link
Member Author

@superstar54 it is :) The failed tests are due to a chain of yet-to-be-merged PRs all the way back to aiidalab/aiidalab-widgets-base#624, which @danielhollas is reviewing.

@superstar54
Copy link
Member

When reviewing this PR, do I need to switch aiidalab/aiidalab-widgets-base#624?.

@edan-bainglass
Copy link
Member Author

If you're testing locally, then yes.

@edan-bainglass
Copy link
Member Author

Need to implement selenium tests to make sure the widget works properly

@edan-bainglass
Copy link
Member Author

@superstar54 @danielhollas rebased on top current state. Please give it a quick look. @superstar54 let me know if I can remove the blocked label.

Copy link
Contributor

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

Just a super quick thoughts.

Thanks on working on this @edan-bainglass looks super intriguing. Unfortunately this week is super busy and on Friday I am leaving for two week holiday. But I am looking forward to delving into this work once I get back.

src/aiidalab_qe/common/infobox.py Show resolved Hide resolved
src/aiidalab_qe/common/infobox.py Show resolved Hide resolved
src/aiidalab_qe/app/wrapper.py Outdated Show resolved Hide resolved

Parameters
----------
`guide_id` : `str`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is quide_id currently used anywhere? What is its purpose?

The name of this is confusing, since below it is added to extra classes, so it's not really an id in a CSS sense.

Also, how is the uniqueness enforced. Is it up to the programmer to ensure that these ids are unique in the whole app? Not sure if that is scalable (especially given that there are also external plugins that supposedly could use this as well perhaps?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The guide id class is used by JS code to toggle the guide. Suppose you were making a new guide for bands. You'd add these widgets all around with the guide id "bands". You would then add a new guide item in the guides list with the key "bands". Selecting the guide bands will toggle all InAppGuides with the bands id.

Now, regarding collisions with existing classes, I'd have to think about it. If someone introduced the bands class, it might override, or unintentionally add styles. Will think and let you know.

Copy link
Member Author

Choose a reason for hiding this comment

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

Naively, I'd enforce that the guide id to be of a certain "unique" format, but I suppose one could always think of a case where that may be accidently used. Not sure how realistic that assumption is. Happy to consider other suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@danielhollas back to this PR. Unfortunately, I am not sure at the moment how to handle this. It is actually not an id, so I'll be changing it to guide_class. It needs to be a class (and not an id), because multiple widgets will share this guide class, such that they can be toggled in unison. I think it is okay for now.

As for uniqueness, I can maybe append -guide to the guide class, so for example bands-guide-identifier. This should help a bit. I can make it more unique of course, e.g. this-is-a-super-unique-guide-class-identifier-for-bands.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is actually not an id, so I'll be changing it to guide_class

Makes sense 👍

As for uniqueness, I can maybe append -guide to the guide class, so for example bands-guide-identifier.
I like the idea to append a string to make this a bit more unique to prevent random clashes in the DOM from outside this widget. Perhaps appending -in-app-guide? That's less generic than just "guide" and is consistent with the classname that you're already adding.

However, my concern was more with how to ensure uniqueness within this class itself, e.g. how to prevent programmer from e.g. copy-pasting a snippet that creates an instance of InAppGuide and forgetting to change the guide_class? QeApp is large enough that one could imagine that making sure all instances of this widget have unique name is non-trivial (not to mention if we ultimately choose to move this widget to AWB).

Perhaps for this you could create a class variable to would hold a list of these guide_class identifiers across all instances, and we could than error out in the __init__ if we detect a duplicate? Something like

class InAppGuide(InfoBox):
    unique_class_names: set = {}
    
    def __init__(self, guide_class: str, classes: list[str] | None = None, **kwargs):
        if guide_class in unique_class_names:
            raise ValueError()
        unique_class_names.add(guide_class)

(just an idea, perhaps I am overthinking it)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's interesting. Note that after some discussion, we've decided to pursue a plugin-based approach for these. So a guide for bands will be provided by the bands plugin (along its settings, results, codes, etc.). The guide radio buttons would then be autodetected from entry points. The guide itself would be limited to the components of the plugin. So for bands, the settings panel and results panel(s).

That said, uniqueness should be handled carefully. Will consider your comment 👍

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 nice work!

Could you add a section on how to add in-app guide in the docs?

@edan-bainglass
Copy link
Member Author

@superstar54 will do 👍

@edan-bainglass edan-bainglass force-pushed the in-app-guides branch 2 times, most recently from e11d121 to 7fff3c3 Compare October 22, 2024 13:46
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.

3 participants