-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Improve online runner check #35722
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: main
Are you sure you want to change the base?
Improve online runner check #35722
Conversation
|
I actually missed a placeholder job log of all queued (waiting) jobs that contains the actual runner label. The advantage over could be run-list performance, on the other hand it is also nice to not need to go into the job list at all. |
Maybe we can add this warning to the job list later as well. |
| <span data-tooltip-content="{{$errMsg}}"> | ||
| {{svg "octicon-alert" 16 "text red"}} | ||
| </span> | ||
| {{end}} |
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 icon is slightly off-center vertically. I think we can potentially fix it by adding flex-text-block to the flex-item-body element. The icon in the sidebar would need similar treatment as per your screenshot.
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.
I tried adding flex-text-block to flex-item-body but it does not work. In 29652f7, I added two span to vertically center the icons but I'm not sure if this is the correct solution.
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 better, but still like 1px too high, but I guess that's acceptable.
One last request: Can you add like tw-ml-2 to the elements so they have a bit more space from the text?
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.
No tw-ml-2, correct flex-text-block should be able to correctly add the gap
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.
Yep, probably best to make a structure like this:
<div class="flex-text-block">
<span>text</span>
<span><svg/></span>
</div>That may likely also solve the icon vertical centering.
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.
Fixed in 07fee46
I used this structure:
<span class="flex-text-block" data-tooltip-content="{{$errMsg}}">
{{svg "octicon-alert" 16 "text red"}}
</span>
But I don't see the gap change. Maybe I'm still incorrectly using flex-text-block and flex-text-inline, or I need to add tw-ml-2?
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.
I think it's worth to spend some time to learn some frontend layouts, how the "display" works, how "flex" works.
If you need to use "align-items: center", then the children nodes should also have "flex" related layout.
The rule is: flex every SVG parent if you used it to align.
<div class="flex-text-block">
<span>text</span>
<span class="flex-text-inline"><svg/></span>
</div>
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.
Thanks for pointing out this rule. Fixed in fb296a3. I realize that I should use <div class="flex-text-block"> to wrap all the children nodes instead of just the SVG parent. Now the gap and vertical centering should be corrent, see the following screenshot:
8406d10 to
fb296a3
Compare

This PR moves "no online runner" warning to the runs list.
A job's
runs-onmay contain expressions likeruns-on: [self-hosted, "${{ inputs.chosen-os }}"]so the value ofruns-onmay be different in each run. We cannot check it through the workflow file.Screenshots
Before:
After:
This PR also splits
prepareWorkflowDispatchTemplatefunction into 2 functions:prepareWorkflowTemplateget and check all of the workflowsprepareWorkflowDispatchTemplateonly prepare workflow dispatch config forworkflow_dispatchworkflows.