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

Add components for dropdown items #8827

Merged
merged 24 commits into from
Mar 14, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jan 3, 2024

image

Background

Currently to create a dropdown in Jenkins you'd do something like:

<l:overflowButton text="My dropdown">
  <a class="jenkins-dropdown__item" href="my-url" rel="noopener noreferrer" target="_blank">
    <div class="jenkins-dropdown__item__icon">
      <l:icon src="symbol-external" />
    </div>
    ${%Website}
  </a>
</l:overflowButton>

The upside of this approach is that you're not restricted as to what you put in your dropdown, e.g. you can display an image or video in your dropdown if you so wish.

The downsides of this however:

  • Inconsistencies can arise between how developers implement dropdowns
  • There isn't currently an easy way of having submenus in dropdowns
  • There's an inconsistency between how model-link dropdowns work (they're powered via JSON rather than Jelly)

My proposal would be to have a set of components, e.g. dd:item, dd:heading, dd:separator, to be a standardised way of implementing such dropdowns. The components themselves are just simple wrappers of template elements, which are converted via JS into HTML to be consumed by Tippy. This gives us the advantage that our implementations of dropdowns are standardised and that our JS API for dropdowns is consistent between Jelly dropdowns and model-link JSON dropdowns.

<template data-dropdown-type="ITEM"
          data-dropdown-id="${attrs.id}"
          data-dropdown-icon="${icon}"
          data-dropdown-text="${attrs.text}"
          data-dropdown-href="${attrs.href}"
/>

To allow for custom content in dropdowns I've added a dd:custom component which will display its child contents in the dropdown, its content needs to be escaped unlike the above components.

As part of this change to demonstrate submenus I've updated the dashboard/build history to use a dropdown, which has a submenu for RSS feeds.
image

Let me know your thoughts 👍

Testing done

  • Tried existing dropdowns in Jenkins, all appear to work correctly
  • Dashboard dropdown works, displays icon legend and submenu as expected
  • There are no dropdown usages in plugins yet so now seems a good time to cement the API.

Proposed changelog entries

  • Add components for dropdown items. Refer to the new Design Library Dropdowns page for implementation details.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@jenkinsci/sig-ux

Before the changes are marked as ready-for-merge:

Maintainer checklist

@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise needs-security-review Awaiting review by a security team member labels Jan 3, 2024
@timja timja requested a review from a team January 3, 2024 22:01
@mawinter69
Copy link
Contributor

mawinter69 commented Jan 4, 2024

The overflowButton tag could also need more documentation

@mawinter69
Copy link
Contributor

mawinter69 commented Jan 4, 2024

Another thing would be to be able to provide a url from where you can fetch json or a callback that can generate the html. That way you can dynamically update the menu without a page reload. As the dropdown js things are not exposed to plugins, I basically had to reimplement it in customizable header plugin, also creating my own webpack so I can access tippy.

@janfaracik
Copy link
Contributor Author

Another thing would be to be able to provide a url from where you can fetch json or a callback that can generate the html. That way you can dynamically update the menu without a page reload. As the dropdown js things are not exposed to plugins, I basically had to reimplement it in customizable header plugin, also creating my own webpack so I can access tippy.

Definitely a good idea. There's the existing model-link stuff which could probably be extended to support that functionality.

@daniel-beck daniel-beck self-requested a review January 4, 2024 14:17
Copy link
Contributor

@Kevin-CB Kevin-CB left a comment

Choose a reason for hiding this comment

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

Looks fine security wise

@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jan 15, 2024
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.

All looks good, not tested locally, could you update the design library though please?

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

The custom tags look a lot better than the HTML, and a new taglib seems like a good location 👍

Re no changelog, this should have at least a Developer: entry.

war/src/main/js/components/dropdowns/templates.js Outdated Show resolved Hide resolved
war/src/main/js/components/dropdowns/utils.js Show resolved Hide resolved
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

See previous review

@janfaracik
Copy link
Contributor Author

Anybody have any other blockers with this PR or is it good to go?

@timja timja requested a review from daniel-beck March 9, 2024 12:08
@@ -47,7 +47,7 @@ function menuItem(options) {
const tag = itemOptions.type === "link" ? "a" : "button";

const item = createElementFromHtml(`
<${tag} class="jenkins-dropdown__item" href="${itemOptions.url}">
<${tag} class="jenkins-dropdown__item ${itemOptions.clazz || ""}" ${itemOptions.url ? `href="${itemOptions.url}"` : ""} ${itemOptions.id ? `id="${itemOptions.id}"` : ""}>
Copy link
Member

Choose a reason for hiding this comment

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

Should be xmlEscape'd (probably including the URL even though it's not new here).

@timja
Copy link
Member

timja commented Mar 11, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 11, 2024
@NotMyFault NotMyFault merged commit 730c59f into jenkinsci:master Mar 14, 2024
17 checks passed
@MarkEWaite MarkEWaite added skip-changelog Should not be shown in the changelog developer Changes which impact plugin developers rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted skip-changelog Should not be shown in the changelog labels Mar 18, 2024
@basil
Copy link
Member

basil commented May 3, 2024

Caused jenkinsci/pipeline-graph-view-plugin#391

@janfaracik janfaracik deleted the revamp-dropdowns branch May 3, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants