Skip to content

Update Workflow State icons#9200

Merged
agrare merged 1 commit intoManageIQ:masterfrom
kbrock:state_logos
Jun 5, 2024
Merged

Update Workflow State icons#9200
agrare merged 1 commit intoManageIQ:masterfrom
kbrock:state_logos

Conversation

@kbrock
Copy link
Member

@kbrock kbrock commented Jun 4, 2024

Before

Icons were working for Tasks
Icons were only displaying play for all others

Monosnap ManageIQ: Requests 2024-06-04 17-00-26

After

Icons are displaying for all Tasks

Monosnap ManageIQ: Requests 2024-06-05 09-43-55

@kbrock
Copy link
Member Author

kbrock commented Jun 4, 2024

update:

  • removed commented out code

if (item.FinishedTime) {
if (item.Output && item.Output.Error) {
if (item.RetryCount) {
// TODO:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the TODO for?

return { icon: 'carbon--MisuseOutline' }
}
} else {
return { icon: 'carbon--CheckmarkOutline', tooltip: 'yay' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think we need this tooltip.

@agrare
Copy link
Member

agrare commented Jun 4, 2024

@kbrock do you have a TLDR for why it wasn't working before? What was it about tasks that was causing it to work? I'm assuming it is because it used to check for RunnerContext but just making sure that's what the real issue was

if (item.Output && item.Output.Error) {
if (item.RetryCount) {
// TODO:
return { icon: 'carbon--RetryFailed', tooltip: `${item.Output.Error} : ${item.RetryCount}` }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tooltip wise, I'm concerned the error will be too big for a tooltip. If there is no error, will it show : 4? Also there's no indicator that the trailing number is the retry count.

@Fryguy
Copy link
Member

Fryguy commented Jun 4, 2024

@kbrock Can we take out the tooltip stuff and do that in a separate PR? I think fixing the icons can be it's own thing.

@Fryguy
Copy link
Member

Fryguy commented Jun 4, 2024

Again, can we remove the tooltips / titles entirely from this PR? I'd like to merge the bug fix separate from the enhancement.

I'm good with the new icon for retry failed.

@kbrock
Copy link
Member Author

kbrock commented Jun 4, 2024

@agrare we were looking at State.RunnerContext.Error. Now were looking at State.Error (and friends)
@Fryguy All set now

update:

  • removed s/yay/boo/g
  • added tooltips
  • removed TODOs

update:

  • fixed color for retry
  • removed tooltips (sorry - missed you in flight)

/paired with @GilbertCherrie


Dev notes:

        let title = item.Output.Cause ? `${item.Output.Error} : ${item.Output.Cause}` : item.Output.Error;
        if (title.length > 100) {
          title = title.substring(0, 100)
        }
        const title = sprintf(__(`Retry Count: %d`), item.RetryCount);

@kbrock kbrock added the wip label Jun 5, 2024
@kbrock kbrock changed the title Update Workflow State icons [WIP] Update Workflow State icons Jun 5, 2024
@kbrock
Copy link
Member Author

kbrock commented Jun 5, 2024

wip: I need to test the just failing case.

Think the retry logic should be bubbled up one level.
Quickly checking and will be all set to merge soon

Before
======

Icons were working for Tasks
Icons were only displaying play for all others

After
=====

Icons are displaying for all Tasks
@miq-bot
Copy link
Member

miq-bot commented Jun 5, 2024

Checked commit kbrock@bbf70d8 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@kbrock
Copy link
Member Author

kbrock commented Jun 5, 2024

update:

  • fixed: failures now display as failure (was success)
  • fixed: retry successes now displays as success (was displaying as a failure)

un-wip: resolved

@kbrock kbrock removed wip labels Jun 5, 2024
@kbrock kbrock changed the title [WIP] Update Workflow State icons Update Workflow State icons Jun 5, 2024
@agrare agrare merged commit e5d7cb2 into ManageIQ:master Jun 5, 2024
@agrare agrare assigned agrare and unassigned GilbertCherrie Jun 5, 2024
@Fryguy
Copy link
Member

Fryguy commented Jun 5, 2024

Backported to radjabov in commit 0301d74.

commit 0301d74f503a42cd2f730a3d42c60ab0ebcced15
Author: Adam Grare <adam@grare.com>
Date:   Wed Jun 5 10:28:14 2024 -0400

    Merge pull request #9200 from kbrock/state_logos
    
    Update Workflow State icons
    
    (cherry picked from commit e5d7cb22689b581662a09917a24006167f247a4c)

Fryguy pushed a commit that referenced this pull request Jun 5, 2024
Update Workflow State icons

(cherry picked from commit e5d7cb2)
@kbrock kbrock deleted the state_logos branch June 6, 2024 00:18
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.

6 participants