Skip to content
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

change color for changed tasks (before not different from not-changed) #571

Merged
merged 5 commits into from
May 25, 2024

Conversation

AntonOvseenko
Copy link
Contributor

@AntonOvseenko AntonOvseenko commented Apr 17, 2024

image

I suggest changing the task-changed display color for better readability of states

@AntonOvseenko AntonOvseenko marked this pull request as draft April 17, 2024 12:58
@AntonOvseenko AntonOvseenko marked this pull request as ready for review April 17, 2024 14:46
@erwindon erwindon self-assigned this Apr 17, 2024
@erwindon
Copy link
Owner

This should always have been the color of these "changed" tasks, as that is also the color of the tasks itself, both in SaltGUI and in salt '*' state.highstate.
I will take a few days to re-organise the code for this all.
The plan is to use a single set of css classes that can be applied.

In the image that you provided, the black-circle and black-circle-with-outline characters look to have equal size.
That is unexpected. did you use any additional tricks?

@AntonOvseenko
Copy link
Contributor Author

No, I change only files from pull request. I reverted my changes to master branch and make new screenshots. Circles have different sizes in highstate page but have same size in job page
image
image

@erwindon
Copy link
Owner

erwindon commented Apr 20, 2024

I rewrote your change a little bit and added the update as a new commit to your branch.
Can you verify that the intended improvement still works fine?

I cannot reproduce the equal size of the characters in Firefox/Chrome/Edge.
Can you share details of your OS+browser?

One small proposal... how about changing circle-with-outline(25C9) with diamond(25C6)?
Just update Character.js to see for yourself.

@erwindon erwindon assigned AntonOvseenko and unassigned erwindon Apr 20, 2024
@AntonOvseenko
Copy link
Contributor Author

AntonOvseenko commented Apr 23, 2024

@erwindon
My main browser is Arc (https://arc.net) and OS - Mac OS Sonoma 14.4.1
I tested changes, looks good (with diamond symbol). Only one moment - if state changes with error it shown in detail page as red diamond (pic. 1), but in highstate page as red circle (pic. 2)
image

image

@AntonOvseenko AntonOvseenko removed their assignment Apr 25, 2024
@erwindon erwindon self-assigned this Apr 25, 2024
@erwindon
Copy link
Owner

@AntonOvseenko
thx for the report!
I'll keep the diamond character; and fix the reported combination.

@erwindon erwindon force-pushed the task-changes-change-color branch 2 times, most recently from 580946f to b232d42 Compare April 27, 2024 18:20
@erwindon
Copy link
Owner

I'm making this a bit more consistent and removed some duplicate code.
And then I found this...
The standard output of a highstate task is to prefix each task with either HEAVY-CHECK-MARK(✔️) or with HEAVY-BALLOT-X (✘).
But using the same symbols (BLACK-CIRCLE=● or BLACK-DIAMOND=◆), and the same colors, as in the summary row is less confusing and more consistent.

@erwindon erwindon force-pushed the task-changes-change-color branch 5 times, most recently from 140883b to 1583d1c Compare May 4, 2024 20:36
@erwindon
Copy link
Owner

erwindon commented May 4, 2024

@AntonOvseenko
I made the intended changes wrt task-symbols:

  • color should now always be ok
  • symbol should now always be ok
  • helper functions are added to determine color/symbol and these are used as much as possible
  • tooltip is added for task-symbols also when summary-mode is used
  • job view for highstate jobs now uses the same symbols per task (was circle/checkmark)

While working on this, I found that results from jobs that use node-groups as target are not always sorted on their minion-id.
I fixed that on the main branch.

Can you please test the version from this branch?

@erwindon erwindon assigned AntonOvseenko and unassigned erwindon May 4, 2024
@erwindon
Copy link
Owner

@AntonOvseenko ping...

Repository owner deleted a comment from sonarcloud bot May 23, 2024
Copy link

sonarcloud bot commented May 25, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@erwindon erwindon merged commit 863904e into erwindon:master May 25, 2024
3 of 4 checks passed
@erwindon
Copy link
Owner

(no response from @AntonOvseenko)
@AntonOvseenko thx for the improvements!
reviewed/tested the changes again and merged it

@AntonOvseenko
Copy link
Contributor Author

(no response from @AntonOvseenko) @AntonOvseenko thx for the improvements! reviewed/tested the changes again and merged it

Sorry, I was on vacation in May and missed a notification from github. I tested the changes, everything works, thanks
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants