Skip to content

Update node symbols#578

Merged
timja merged 57 commits intojenkinsci:mainfrom
janfaracik:node-buttons
Feb 5, 2025
Merged

Update node symbols#578
timja merged 57 commits intojenkinsci:mainfrom
janfaracik:node-buttons

Conversation

@janfaracik
Copy link
Member

@janfaracik janfaracik commented Feb 2, 2025

Raised at the recent Contributor Summit, it'd be cool if we could CMD click nodes in the graph. This PR does just that, as well as a few tidy ups along the way.

What's changed?

  • Nodes are now links, meaning they're more accessible, focus navigable and CMD clickable
  • We now use symbols from Jenkins core
  • Minor tidy ups

Before
image

After
image

Testing done

  • Tried with a basic pipeline, looks fine.
  • If there's test data I can use I'm happy to try that.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@janfaracik janfaracik marked this pull request as ready for review February 2, 2025 10:34
@janfaracik janfaracik requested a review from a team as a code owner February 2, 2025 10:34
@timja timja added the enhancement New feature or request label Feb 2, 2025
…ort/nodes.tsx

Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
@timja
Copy link
Member

timja commented Feb 2, 2025

Stage view is broke (multi-pipeline-graph) Click stages from the job. Can also be shown on the job page if enabled on the appearance page.

@janfaracik
Copy link
Member Author

Stage view is broke (multi-pipeline-graph) Click stages from the job. Can also be shown on the job page if enabled on the appearance page.

Fixed now -

image

@mikedld
Copy link
Contributor

mikedld commented Feb 2, 2025

On the screenshots, the new icons look slightly misaligned vertically, shifted to the top.

@janfaracik
Copy link
Member Author

On the screenshots, the new icons look slightly misaligned vertically, shifted to the top.

Good spot, fixed.

@timja
Copy link
Member

timja commented Feb 2, 2025

404 when clicking on a stage from either the job page or the Stages page

@janfaracik
Copy link
Member Author

          const currentPath = this.props.path;
          let nodeUrl = "";

          if (currentPath) {
            const runId = currentPath.split("=")[1];
            if (currentPath.startsWith("multi-pipeline-graph/")) {
              nodeUrl = `${runId}/pipeline-console?selected-node=${node.id}`
            } else {
              nodeUrl = `../${runId}/pipeline-console?selected-node=${node.id}`
            }

            nodes.push({...node, url: nodeUrl});
            continue;
          }

          const newPath = this.getTreePath();
          if (newPath.startsWith("pipeline-graph/tree")) {
            nodeUrl = `pipeline-console?selected-node=${node.id}`
          } else {
            nodeUrl = `../pipeline-console?selected-node=${node.id}`
          }

          nodes.push({...node, url: nodeUrl});

I've put in a very hacky (working) way of pulling the URL - happy to move it to the API (and provide absolute urls in the payload) if thats cleaner?

@timja
Copy link
Member

timja commented Feb 4, 2025

Absolute URLs in the API would be cleaner if not too hard, currently its a pain, source of mistakes and has been broken before.

@janfaracik
Copy link
Member Author

Added a url field to nodes, it's working now. Still feels a little hacky, any thoughts on the implementation?

@timja
Copy link
Member

timja commented Feb 5, 2025

Added a url field to nodes, it's working now. Still feels a little hacky, any thoughts on the implementation?

Seems fine, can you take a look at the build failure please?

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

Re-tested all LGTM! Thanks

@timja timja merged commit 98f212e into jenkinsci:main Feb 5, 2025
17 checks passed
@janfaracik janfaracik deleted the node-buttons branch February 5, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants