-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
DEV UI Enhancing the Scores Page in RESTEasy Reactive #15384
Conversation
I'll let @phillip-kruger follow up on this since I don't know anything about CSS nor do I have any artistic talent :) |
So, Interestingly it took only 15 minutes to understand accordion and another 15 to implement it on a separate project - https://github.com/SaumyaSingh1/accordian-demo Right now after adding accordion, my page is like this : I am liking frontend work :) |
I like it ! Better than what we have at the moment, right ? |
Yeah, I am now doing my research for the gauge part :) |
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.
LGTM
I think it's a bit of a shame we lose the color indicator in the main view? It was a good visual hint. |
I will add that, it is not final yet. It is on the to-do! |
Yea maybe make the percentage in color ? |
of course, this is the plan. |
And even if that will not look much cool so we will put that in the badge inside |
Also, don't forget to update https://quarkus.io/guides/dev-ui when you are done |
sure Today was looking at the Prod issue, understood Jenkins flow 🙈 so didn't get time to work on this :) will get back here maybe tomorrow or a day after that. |
No rush at all. Yeah, when you do update the docs, include all updates |
gauge.querySelector(".gauge__cover").textContent = `${Math.round(value * 100)}%`; | ||
} | ||
|
||
setGaugeValue(gaugeElement, \{diagnostic.score}/100); |
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 line @mkouba
Replacing {diagnostic.score}
with numeric value is working fine.
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.
Ok, so you can either remove the unparsed character data section, i.e. {|
and |}
and escape the first bracket for {value / 2}
and {Math.round(value * 100)}
- we need to ignore these during rendering:
const gaugeElement = document.querySelector(".gauge");
function setGaugeValue(gauge, value) {
if (value < 0 || value > 1) {
return;
}
gauge.querySelector(".gauge__fill").style.transform = `rotate($\{value / 2}turn)`;
gauge.querySelector(".gauge__cover").textContent = `$\{Math.round(value * 100)}%`;
}
setGaugeValue(gaugeElement, {diagnostic.score}/100);
Alternatively, you can end the unparsed character data section right before the {diagnostic.score}
expr. And of course, you can't escape the {diagnostic.score}
expr because you need to output the value (not ignore ;-).
{| <script>
const gaugeElement = document.querySelector(".gauge");
function setGaugeValue(gauge, value) {
if (value < 0 || value > 1) {
return;
}
gauge.querySelector(".gauge__fill").style.transform = `rotate(${value / 2}turn)`;
gauge.querySelector(".gauge__cover").textContent = `${Math.round(value * 100)}%`;
}
|}
setGaugeValue(gaugeElement, {diagnostic.score}/100);
BTW I noticed several JS errors in the console:
Uncaught SyntaxError: redeclaration of const gaugeElement <anonymous> http://localhost:8080/q/dev/io.quarkus.quarkus-resteasy-reactive/scores:247
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.
And FYI we now throw an exception if a property is not found in an expression, just rebase this PR on master ;-).
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.
perfect this is working
I did the alternative way you suggested :)
Uncaught SyntaxError: redeclaration of const gaugeElement http://localhost:8080/q/dev/io.quarkus.quarkus-resteasy-reactive/scores:247
Where you caught this, it's not showing on running mvn quarkus:dev
nor in logs.
But yes I got the point, the JS snippet is in for
loop and for each endpoint
it is getting declared and do you know the JS snippet is working only for 1st time. after that I think it stops due to redeclaration of const gaugeElement
I think I need to declare const gaugeElement = document.querySelector(".gauge");
globally somewhere
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.
Where you caught this, it's not showing on running mvn quarkus:dev nor in logs.
In the browser console, it's a JS error ;-)
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.
Yesterday I tried several gauge designs, and finally decided to use this https://github.com/SaumyaSingh1/Responsive-Gauge-Pure-CSS. It has little bit of JS involved else pure CSS. I guess I need to give more finishing in terms of margins, colors and styling but here is how the page is rn : As you will observe, the Gauge is showing Full Green for 100% in only the 1st category which is correct. But for other gauges perhaps the JS Snippet is not working. well, it is working in the side project I made for gauge yesterday. Maybe I will need some help with the JS part :) |
I am feeling like I have got too much Web Dev knowledge now. we learn more when we do things practically 🙈 |
Looking nice! This may be a dump question: Why are the second and third gauges at 100% but not "fully loaded"? |
No this is the right question, this is what I explained above that screenshot Writing here again :) As you will observe, the Gauge is showing fully loaded for 100% in only the 1st category which is correct. But for other gauges perhaps the JS Snippet is not working. well, it is working in the side project I made for gauge yesterday. |
And perhaps this conflict is due to #15406 that we merged in master. Tried manually resolving conflicts but ig I did something wrong |
Fixing git conflicts is definitely an adventure the first few times :). After you've done it, it becomes easier :) |
</div> | ||
|
||
{| <script> | ||
var gaugeElement = document.querySelector(".gauge"); |
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.
JS snippet is working fine, but only the 1st Gauge is loaded correctly, as we can see in this picture as well.
If there is a simple mistake I am making here, but not able to figure it out? @FroMage @phillip-kruger
I am sure this is not Qute issue.
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.
Even in the personal demo project for Gauges, when I am adding for
loop around the gauge part, other gauges(except 1st) are not getting correctly loaded.
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.
Sorry I had replied yesterday but forgot to click send :(
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.
ohh, no problem at all :)
I'll have a look a.s.a.p |
return; | ||
} | ||
|
||
gauge.querySelector(".gauge__fill").style.transform = `rotate(${value / 2}turn)`; |
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.
So I can tell you here that the issue is that this JS script is in a loop, so it will end up multiple times in the generated HTML. So you have multiple global variables called gaugeElement
and setGaugeValue
, which should not be a problem in JS, but the query selector you're doing gets called multiple times, and every time you call it, it applies to every element, not just this one.
So there are several ways to do this. My favourite is to set the diagnostic.score
in a data
attribute somewhere, and then use it in JS in a global single query call.
Something like:
<div class="gauge" data-score="{diagnostic.score}">
And then a single selector script pass at the end of the page:
for(var gauge in document.querySelectorAll(".gauge")){
setGaugeValue(gauge, gauge.getAttribute("data-score"));
}
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.
then a single selector script pass at the end of the page
End of the page meaning after all the for
loops and just above the body
tag 🤔
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, end of the body.
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.
Ok
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.
The best way to include a custom script for the DevUI is to do something akin to : https://github.com/quarkusio/quarkus/blob/master/extensions/vertx-http/deployment/src/main/resources/dev-templates/io.quarkus.quarkus-vertx-http/config.html#L27
<tr> | ||
<td style="width:30%;">Filters</td> | ||
<td style="width:70%;"> | ||
<span class="badge badge-light" id="collapse-list" data-toggle="collapse" data-target="#filter"> |
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.
Can we make the entire row clickable for expand/collapse? I hate tiny >
buttons :)
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.
sure :)
{#for diagnostic in diagnosticEntry.value} | ||
<div class="col-4"> | ||
<div class="row"> | ||
<div class="gauge"> |
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.
@FroMage
Here I already have a gauge class so is it a good idea to put the data-score
here like :
<div class="gauge" data-score="{diagnostic.score}">
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.
Sure.
<td style="width:70%;"> | ||
<span class="badge badge-light" id="collapse-list" data-toggle="collapse" data-target="#filter"> | ||
<td style="width:70%;" id="collapse-list" data-toggle="collapse" data-target="#filter"> | ||
<span class="badge badge-light"> |
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.
Now with this new commit, the complete row is clickable :) But I didn't remove the icon >
, should I @FroMage? I was not sure if a blank row will look nice :)
{/for} | ||
<script> |
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.
@FroMage I used the method of data
attribute to fix that Gauge Loading issue but it's not giving desired results 🤔
return; | ||
} | ||
|
||
gauge.querySelector(".gauge__fill").style.transform = `rotate(${value / 2}turn)`; |
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.
Well you need to get rid of this, as it will conflict with your end-of-body script
Hi @SaumyaSingh1 - can you share your example app (the one that you took the screenshots from) |
Hi @phillip-kruger, sure here is the separate repo that I used for Gauge Part - https://github.com/SaumyaSingh1/Responsive-Gauge-Pure-CSS The screenshots of the score page are from this piece of code only that I am committing to branch |
But you are using some Quarkus app to test against that branch ? That is what I am after :) |
oh, I see you are asking for the quarkus app the one I named as |
@phillip-kruger Here is the Quarkus App - https://github.com/SaumyaSingh1/QuarkusDemoApp |
Thanks. Looking at it now. |
This commit gave a beautiful finishing touch to the Scores Page 🎉 The changes helped me understand what better I could have done, |
Hi, With these commits I did the following :
Can we have a final review for the Scores 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.
Looks much better, thanks both of you!
Looks good !! |
7e4f429
to
b36ad35
Compare
DEV UI: added accordion DEV UI: added gauge in scores screen, not final added gauge for scores part added gauge code DEV UI: added accordion, gauge filter complete row clickable, fixing gauge loading part Suggested changes to score page. Signed-off-by:Phillip Kruger <phillip.kruger@gmail.com> Fix chrome issue: use Transform Filter Name: Removed unnecessary data Added Different Colors to Gauge based on score value, Tested
I force pushed to relaunch CI. |
This is draft PR for issue #15322 :)
CC @FroMage @geoand @phillip-kruger