-
Notifications
You must be signed in to change notification settings - Fork 235
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
Move UI visualizer code out of this repo #1146
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1146 +/- ##
==========================================
- Coverage 76.87% 76.59% -0.29%
==========================================
Files 390 382 -8
Lines 61889 60822 -1067
Branches 11376 11200 -176
==========================================
- Hits 47576 46584 -992
+ Misses 11857 11822 -35
+ Partials 2456 2416 -40
☔ View full report in Codecov by Sentry. |
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 looks OK to me. My only comment is that we should probably be adding some sort of pointer to documentation on how to install and set up the UI in the main IDAES docs.
@andrewlee94 w.r.t. installation instructions I agree, and it was on my to-do list. In a perfect world that would go first. But I have noticed that the current version in main has a bug in the stream table, that didn't seem to lead to any complaints (but looks bad). So in the interest of simplicity, I'd like to just move forward with this PR and then add back instructions once I've got a less borked version of the UI working in the new repo. |
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.
I've left a suggestion to consider whether we need the added complexity of the wrapper class for this particular case since there's only one call site to handle. Otherwise looks good.
I agree with the changes. I think it's a good idea to remove the UI from the main code and add it to its own repo. This will make the code more modular and easier to maintain. I also agree with Andrew's comment that we should add some sort of pointer to documentation on how to install and set up the UI in the main IDAES docs. This will make it easier for users to get started with the UI. Or we can make some automation install when they install IDAES. |
|
Waiting on @dangunter and/or @lbianchi-lbl to fix the git conflicts. |
Fixes #1129
Related to IDAES/idaes-ui#11
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: