ui-tooltip and circle changes made in graph for waived nodes and profiles #4456
Conversation
|
Deploy preview for chef-automate ready! Built with commit 5a4cf01 |
|
Hi @vivekshankar1, is this PR related to this ticket: #4382? It looks like they may be. If so, I will connect them 👍 |
Thanks @SEAjamieD, yes this is the issue ticket. I did not have access to chef repo till now(hence the pr from forked branch), got access just now. Should we send the pr again by directly pushing to the official automate remote? |
Signed-off-by: Vivek Shankar <vshankar@progress.com>
…ekshankar1/automate into vivek/waived_nodes_and_profile_on_ui
Awesome! Thanks for tackling this. I've linked this PR to the #4382 so that its tracked until we decide what our new ticketing system will be. I'm not sure if you'll need to create a new PR, @msorens would you be able to address this question? Vivek's PR is off a forked repo - so you know if it is necessary to open a new PR? |
|
Should not be a problem to merge this PR once it is reviewed and approved. Should there be any problem when we attempt that, we can always regenerate the PR afresh at that time. |
There was a problem hiding this comment.
Everything is looking good so far as I can tell from the code alone. I'm working on getting this spun up now, but am having issues I think because its from a fork.
I'm going to hold off on approving until I can get the branch running - but from this initial pass its looking good. 🎉
| if (d.failed === d.passed && d.failed === d.skipped && d.failed === d.waived) { return 10; } | ||
| if (d.failed === d.passed && d.failed === d.skipped) { return 8; } | ||
| if (d.failed === d.passed || d.failed === d.skipped) { return 6; } | ||
| if (d.failed === d.passed && d.failed === d.waived) { return 8; } | ||
| if (d.failed === d.skipped && d.failed === d.waived) { return 8; } | ||
| if (d.failed === d.passed || d.failed === d.skipped || d.failed === d.waived) { return 6; } | ||
| return 4; | ||
| }); | ||
|
|
||
| update.select('.status-dot.passed') | ||
| .attr('r', d => d.passed === d.skipped ? 6 : 4); | ||
| .attr('r', d => { | ||
| if (d.passed === d.skipped && d.passed === d.waived) { return 8; } | ||
| if (d.passed === d.skipped || d.passed === d.waived) { return 6; } | ||
| return 4; |
There was a problem hiding this comment.
Out of curiosity - what is the reason here for returning 10, 8, 6, or 4?
There was a problem hiding this comment.
Hi @SEAjamieD ,
As there are 4 circles now (max value increased from 8 to 10), each circle will take 2 units and base circle default was 4.
That makes 4+2+2+2 = 10
if there are 3 circles then, 4+2+2 = 8
if there are 2 circles then, 4+2 = 6
if its only 1 circle then 4
There was a problem hiding this comment.
Thanks @vivekshankar1! I appreciate the education 👍
There was a problem hiding this comment.
@vivekshankar1 I want to urge a follow-up that makes the code much less obfuscated. As it is, it is very challenging to figure out what it is doing, but since you have already done the figuring, you are at an ideal point to explain it. 😁
I would consider replacing the magic numbers with named constants. Also, it needs some comments. I would start with explaining what the block of update.select('.status-dot.failed') is supposed to be doing, as I have no idea.
(Thanks for pointing that out, @SEAjamieD )
There was a problem hiding this comment.
Hi @msorens , i have added comments and the requested changes. If this is good i can send another PR
https://github.com/vivekshankar1/automate/blob/02ffd2f502532364816990a746087b3afb5fcb4a/components/automate-ui/src/app/pages/%2Bcompliance/%2Breporting/%2Breporting-overview/overview-trend/overview-trend.component.ts#L26-L31
msorens
added
acceptance: verified
acceptance: issue
and removed
acceptance: verified
labels
|
Issue found during acceptance testing: |
🔩 Description: What code changed, and why?
To show the waived nodes and profile data on the compliance graph.
⛓️ Related Resources
OFFR-E-10 #4382
👍 Definition of Done
Changes should be for both node status and profile status tab.
There should be a new waved line in the graph
Waived data should show up in the tooltip on compliance graph.
The intersection dots should accommodate the new waived circle.
👟 How to Build and Test the Change
Add some waived data. You can see the new line and numbers in the tooltip.
✅ Checklist
📷 Screenshots, if applicable