Add info box widgets with CSS stylesheets #623
Conversation
edan-bainglass
force-pushed
the
infobox
branch
9 times, most recently
from
4a1f60a
to
b574b51
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #623 +/- ##
==========================================
+ Coverage 83.61% 83.64% +0.02%
==========================================
Files 16 18 +2
Lines 3522 3595 +73
==========================================
+ Hits 2945 3007 +62
- Misses 577 588 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@superstar54 @unkcpz can you guys have a look at the failed tests? I don't see how my changes could suddenly trigger the following error, which is failing several structure-related tests! |
|
Hi @edan-bainglass , spglib released a new version yesterday. Apparently, it drops support for some old API. |
|
I just created an issue on this: #625 |
|
@superstar54 I see. So we need to update on our end? I take it this is unrelated to my PR. I would rebase on top of an update PR once that's in? Regarding an |
No idea yet. |
|
You can just add the DeprecationWarning to the list of ignored warnings in pyproject.toml (We have configured pytest so that it turns any warnings into errors, maybe we should change it) |
edan-bainglass
force-pushed
the
infobox
branch
2 times, most recently
from
82f066b
to
877c029
Compare
edan-bainglass
force-pushed
the
infobox
branch
from
877c029
to
cff6418
Compare
edan-bainglass
force-pushed
the
infobox
branch
from
cff6418
to
0e85e55
Compare
aiidalab_widgets_base/infobox.py
Outdated
| def __init__(self, **kwargs): | ||
| """`InfoBox` constructor.""" | ||
| custom_css = kwargs.pop("custom-css", "") |
There was a problem hiding this comment.
I would add the custom-css explicitly, so the user knows it quickly.
| def __init__(self, **kwargs): | |
| """`InfoBox` constructor.""" | |
| custom_css = kwargs.pop("custom-css", "") | |
| def __init__(self, custom-css=None, **kwargs): | |
| """`InfoBox` constructor.""" |
There was a problem hiding this comment.
I actually had it like that initially but changed it for what I vaguely recall was a good reason. If I can't remember it in a day or so, I'll revert to this.
There was a problem hiding this comment.
Btw: I think the parameter should be called custom_class. ;-)
There was a problem hiding this comment.
I redesigned this part a bit. I think it is clearer. Let me know.
| def _write_existing_user_token_to_file(self): | ||
| """Write a token to file marking the user as an existing user.""" | ||
| with open(".app-user-config", "w") as file: | ||
| file.write("existing-user") |
There was a problem hiding this comment.
What if I have two FirstTimeUserMessage in my app? They will share the same config file; thus, there is a risk that they are in conflict.
There was a problem hiding this comment.
I agree. This isn't a great approach. Username would be better, or some other uniquely identifying property. This was just a proof of concept. I was hoping someone with more knowledge of AiiDAlab could provide a better solution :)
There was a problem hiding this comment.
Should this even be a backend logic? For example, our aiidalab deployment is currently shared in the group, so you can't really identify new user this way.
Probably more canonical solution is to store a cookie in user's browser.
There was a problem hiding this comment.
I really don't like how I've implemented it. Just needed a proof of concept. Open to suggestions. A cookie to the user's browser may be a good approach.
There was a problem hiding this comment.
@giovannipizzi suggested we keep this as a local config file and had some ideas for its structure. I'll let him explain it.
edan-bainglass
force-pushed
the
infobox
branch
from
0e85e55
to
93d2986
Compare
|
Moving this PR to the QE App for local testing. If all is good and approved for general use, |
Based on #624
This PR adds info box widgets to provide in-app information to users