Conversation
timja
left a comment
There was a problem hiding this comment.
cc @janfaracik and @lewisbirks
|
|
||
| const layout: LayoutInfo = { | ||
| ...defaultLayout, | ||
| nodeSpacingH: 45, |
There was a problem hiding this comment.
needs to be re-added behind a setting, possibly we can calculate it based on longest stage name too so we don't have to lose all the value by shortening it
|
|
||
| return ( | ||
| <div className="pgv-single-run"> | ||
| <div className="pgv-single-run" style={{ height: "100px" }}> |
There was a problem hiding this comment.
needs to be dynamic based on a setting
src/main/frontend/multi-pipeline-graph-view/multi-pipeline-graph/main/single-run.scss
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| .pgw-user-specified-text { | ||
| .pgv-user-specified-text { |
| for (const node of row) { | ||
| measuredWidth = Math.max(measuredWidth, node.x + nodeSpacingH / 2); | ||
| measuredHeight = collapsed | ||
| ? 60 |
There was a problem hiding this comment.
may need re-adding behind setting
| if (collapsed) { | ||
| return []; | ||
| } | ||
| // if (collapsed) { |
There was a problem hiding this comment.
needs to be re-added behind a setting
| ): Array<NodeLabelInfo> { | ||
| const labels: Array<NodeLabelInfo> = []; | ||
|
|
||
| // if (collapsed) { |
There was a problem hiding this comment.
needs to be re-added behind a setting
| left: x + "px", | ||
| }; | ||
|
|
||
| const classNames = ["PWGx-pipeline-big-label"]; |
There was a problem hiding this comment.
could review if all these classes and features are needed, its mostly a copy of big label
src/main/frontend/pipeline-graph-view/pipeline-graph/main/support/nodes.tsx
Show resolved
Hide resolved
|
This is looking great! |
Signed-off-by: lewisbirks <22620804+lewisbirks@users.noreply.github.com>
Signed-off-by: lewisbirks <22620804+lewisbirks@users.noreply.github.com>
Signed-off-by: lewisbirks <22620804+lewisbirks@users.noreply.github.com>
Signed-off-by: lewisbirks <22620804+lewisbirks@users.noreply.github.com>
|
Looks really good. When will it be released? @timja |
Hopefully this week, just had a couple of other things to sort first. |
|
This plugin is great, but there are a few obvious issues.
|
Can you please file these as issues (if they don't exist already) so these can be unpicked to understand what you mean, thanks. |
- Merged settings and console functionality - Kept error handling in useMessages function - Combined localization keys from both branches
|
Thank you. I will review the issues and submit issues after experiencing this PR release. |
|
Happy to do a bit more here if people think its needed, should be good for review now though |
lewisbirks
left a comment
There was a problem hiding this comment.
Looking good and seems to work after some manual testing
src/main/frontend/pipeline-graph-view/pipeline-graph/main/support/labels.tsx
Outdated
Show resolved
Hide resolved
| showNames: boolean, | ||
| showDurations: boolean, |
There was a problem hiding this comment.
would it make sense to combine these with collapse in some sort of DisplayConfig object?
There was a problem hiding this comment.
I'd be a little worried that it complicates getting everything to re-render properly but if you think its worth it I can take a look.
There was a problem hiding this comment.
nah, it's nothing major, was just thinking about potentially grouping logical params into a parent object is all
…ort/labels.tsx Co-authored-by: Lewis Birks <22620804+lewisbirks@users.noreply.github.com>
|
I noticed in #343 it mentions the following
Do we want that? And if so would it be better to split out into it's own issue once this is merged? |
Yeah new issue I think |
I gave it a bit of a go but didn't get something working, would probably be possible to stop the skipped line and just draw a regular one, might be the easiest thing but I don't think its a blocker. |
|
Yeah, it is not a blocker for sure. |
|
I suggest abbreviating all time units like ‘’4min 25 seg to ‘’4m25s’’ so it looks more friendly. |
I agree with this one, but I think it deserves its own issue. It's not related to this. Maybe there should be a toggle to show all stages or not, which can otherwise be problematic for performance reasons. |
|
The stages that do not need to be executed can be hidden, but the stages that are executed are displayed in one view, and you don’t need to scroll to see them. |
I don't think this is a good idea because it would make comparisons harder if different builds skips different stages. |
|
Technical implementation may be difficult, but this is the best user experience, Too many stages are hidden. When I can't see the stage I'm currently building, this view becomes meaningless. |
That's part of #650, which is in the priority epic so will look to improve this soon. |
|
Hello @timja Thanks for the work! Is it possible to not show timings on skipped stages? I assume that skipped stages are not executed, which could save space on the graph. Another question: The configuration to display stage names and their duration is set per user. Is it possible to enable this globally? |
|
Makes sense, I've created issues to track this feedback: |






Fixes #688
Most of these aren't solved yet but will be done with this PR:
Before
After
Testing done
Submitter checklist