-
-
Notifications
You must be signed in to change notification settings - Fork 240
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
Update frontend dependencies #708
Update frontend dependencies #708
Conversation
Hey @melck, BIG thanks for this contribution! It will greatly improve the maintainability of this project. 😊 🥳 🤝 I would be happy to test and review this PR as soon as it's ready. However I do suggest to separate the changes to layout and possibly features (last 2 commits?) to yet another PR 😅 - it will be much faster to merge only the dependencies upgrade and code refactoring changes. |
Hey @melck! Do you need any help with this PR? |
Hey @gdubicki, sorry I've been pretty busy, I'll take care of it before the weekend. |
No problem, I don't want to rush you! I just don't want all your hard work on this to spoil. :) |
57e6782
to
15b5d7f
Compare
Codecov Report
@@ Coverage Diff @@
## master #708 +/- ##
==========================================
- Coverage 85.91% 85.45% -0.47%
==========================================
Files 19 19
Lines 1051 1045 -6
==========================================
- Hits 903 893 -10
- Misses 148 152 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Hello @gdubicki, i have updated this PR. I removed commits for changing layout and fixed unittest of offline statics. Code coverage has reduced because of removing some unused code. I don't know why ... |
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.
Thanks a lot for completing the PR, @melck! I did test it out and I noticed one thing that I think we have to fix and 2 others that we might improve. Please check out my comments.
puppetboard/templates/reports.html
Outdated
@@ -27,7 +27,7 @@ | |||
{% endfor %} |
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.
Nit: I know that it's out of the scope of the PR, but it would be nice to unify this to look and work the same way as such filter in the /nodes
page.
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.
Yes i will do that later. When this PR is ok / merged, i will update my other one. After that i will open a new one for UI unification and improvements.
To be honest, in the long run it would be easier to add a real frontend and only manage json returns on the backend. Remove all JS / CSS dependencies and start from scratch with, for example, the vuejs library and the element-plus framework.
Frontend update management would be simplified and optimized to handle large amounts of data
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 totally agree that the frontend of the app just needs to be replaced with some modern, standard solution. I don't have the skills to do that though, but if you are willing to implement it then it's great and you are welcome to do it! I will do my best to help you test it, review the code, get it merged and released. :)
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 100% agree with @gdubicki . I do not have the skills to write the frontend, but I think its a really good idea. I am happy to review and test PRs.
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.
Perhaps we should sync on Voxpupuli slack regarding this, @melck? If you want, please join it at https://puppetcommunity.slack.com/messages/voxpupuli/ . @bastelfreak and me are there under the same ids as here.
7749792
to
72e82b6
Compare
@melck : I don't want to be a nuisance, but I still see a wrong order of nodes in index. :( The code in the master: v4.0.2: It's not just a matter of changing
desc as then the unreported nodes are on the top, instead of being on the bottom...
|
@melck: sorry to bother you, but I hope that the above issues will be easier to fix for you than for me. And they are blocking the release. |
Sorry i'll wheck that. Do i have to creater another PR ? |
Yes, please create a new PR, @melck. Thank you! |
that has been removed in voxpupuli#708 Fixes voxpupuli#714
that has been removed in voxpupuli#708 Fixes voxpupuli#714
that has been removed in voxpupuli#708 Fixes voxpupuli#714
Upgrade frontend dependencies :
puppetboard/static/libs
layout.html
to_offline_static.html
and_online_static.html
Move to another PR after discussions
@gdubicki This draft PR is from discussion on #705 with some visual changes. What do you think of it ?