-
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
Resource setting tabs #939
Resource setting tabs #939
Conversation
- Find all codes in the plugins - Only list one code for each code kind - Each plugin has a code panel - Modification in basic will update all plugins - Add a override box in each plugin panel - Warning and blocker message - Basic panel only show the Base QEcode class, and plugin decide how to update their custom code widget
28b3af5
to
5f67bc0
Compare
for more information, see https://pre-commit.ci
c1491b8
to
aabefa3
Compare
aabefa3
to
cfe0ae7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
+ Coverage 68.05% 68.40% +0.34%
==========================================
Files 104 110 +6
Lines 5795 6096 +301
==========================================
+ Hits 3944 4170 +226
- Misses 1851 1926 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
1c030f0
to
8245320
Compare
8245320
to
b170550
Compare
Hi @edan-bainglass , please take a look at this PR and let me know if I am on the design following the MVC. Hi @mikibonacci and @AndresOrtegaGuerrero, I created base classes for the code setting model and view. The plugin only needs inheritance from them, making it quite simple for the developer. You can look at the |
Thanks @superstar54. I will look at it this morning.
@mikibonacci @AndresOrtegaGuerrero maybe hold off on implementing these features for your plugins until we review the PR. |
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.
This is great @superstar54! Thank you. As discussed, we'll do a bit of refactoring in a follow-up PR, now that we see the added shared functionality across steps 2 and 4.
Leaving here comments from @mikibonacci:
- Parallelization pool not synching from global resources to plugin resources
- Unchecking the override on a plugin resources panel should resync to global resources
- We should check that adding a new code is reflected correctly in the relevant panels
But in general, LGTM! Let's get this merged!
This PR looks to clean up the recently implemented plugin-based tab system in the resources/submission step (#939). The idea is to leverage the newly developed abstraction in #945 to encapsulate the behavior of ANY resource settings panel, leaving the global resource settings panel to override based on its unique added features. A few added items addressed in this PR: - PW code parallelization widget syncing between tabs - Support for syncing newly setup codes - Warning notification for overridden plugin resources - Syncing back with global resources on un-override
This PR implemented the plugin panel for the resource setting step. The design is agreed by the team, and as show in this diagram:
https://www.figma.com/board/EAwAPebMQLnoexFirWXbCO/qeapp-codes-panel-design?node-id=0-1&node-type=canvas&t=RA169OqQuAXSHmU0-0
The core ideas are:
global settings
currently. But the plugin developer can implement their own logic to update the warning and blocker.Global settings
panel only shows the Base QEcode class, and the plugin decides how to update its custom code widgetPlugin design
MVC base class
I created base classes for the code setting model and view. The plugin only needs inheritance from them, which becomes quick and simple for the developer.
Above
_on_xxx_change
in MVCIn the following example, the
_on_global_codes_change
only acts on the model, so sometimes, it is confusing why we need to move the logic to the setting.Closes #879