-
Notifications
You must be signed in to change notification settings - Fork 5
Fix requests #451
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
base: master
Are you sure you want to change the base?
Fix requests #451
Conversation
|
Thats pretty difficult or almost impossible to review for me :( I just hope it does not break stuff.. I would welcome if we can return to better review practices in the VILLAS repos: Small PRs addressing one issue at a time. |
|
@stv0g this is not meant to be the continous practice. It is unorganized due to being a collection of small fixes to exactly avoid a couple breaking changes. EDIT: if it is ABSOLUTELY an issue, i can squash some |
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
add: missing branding Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
…mponents Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Implement toggle mode Adjust size of the box for the WidgetPlayer fix: missing data after merge/rebase Signed-off-by: Andrii Podriez <andrey5577990@gmail.com> Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
bb0f6e1 to
fb4ea52
Compare
|
@stv0g I squashed commits that have to do with one another. |
|
Hi @SystemsPurge, Sorry, I dont have the bandwidth at the moment to review these larger PRs. @iripiri Do you have some resources to look at this (and the other refactoring/maintenance PRs in VILLASweb)? |
iripiri
left a comment
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.
Hi @SystemsPurge and @Anyandsi,
the changes mostly look good to me!
I've tested the SLEW example in the villas.k8s deployment and had two issues:
a) Job execution fails because of hardcoded URL for results file!
b) there are two waiting symbols/GIFs running/visible which I don't understand
[c) name of component configuration is not visible as it was before, but this is negligible]
I was also confused with the file naming (js/jsx), it would be good to go through the files again and check whether this is really consistent.
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.
Shouldn't this also be .jsx?
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.
We didn't want to make the changes any more unreadable in this review than they already are, if we go around changing all the filenames they would all be tagged and come up here. We are changing them gradually, most importantly in the dependency update 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 a lot nicer than the old version 👍
| signals={signals} | ||
| /> | ||
|
|
||
| {/* <EditFilesDialog |
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.
Why is the Files Dialog greyed out? Does it still need testing? What's the difference between this file and dashboard.jsx, can you add some explanatory comments in the code?
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 one has always been grayed out. I restored it in some other branch and it seems to work. It should have the same functionality as the one in dashboard and will be restored accordingly in a next PR
|
@iripiri |
…ion) Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
Signed-off-by: SystemsPurge <naktiyoussef@proton.me>
A collection of fixes and refactoring changes to make frontend run smoother with redux
Includes refactoring into functional components, and adjustment of some breaking changes that happened with the migration