Revamp icon legend as a modal #7718

janfaracik · 2023-03-12T16:01:27Z

Introduces a new lightweight modal component (currently lacking in depth, but eventually it could be used for confirmation dialogs, notices etc)
Replaces the icon legend page with an accessible modal

Relates to https://issues.jenkins.io/browse/JENKINS-68357

Testing done

Icon legend works the same as before

Proposed changelog entries

Revamp icon legend as a modal

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
- Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

timja · 2023-03-12T16:28:45Z

Looks really nice.

Feedback / questions

Should an overlay be placed on the rest of the page to show you can't interact with it when the modal is open?
Should clicking outside the modal close it? (It currently doesn't)
Contrast isn't sufficient for disabled / aborted on dark theme:

janfaracik · 2023-03-12T16:52:36Z

Should an overlay be placed on the rest of the page to show you can't interact with it when the modal is open?

There is an overlay (it darkens the page)

Should clicking outside the modal close it? (It currently doesn't)

That makes sense to me, especially for a simple modal like this.

Contrast isn't sufficient for disabled / aborted on dark theme:

👍 Just opened a PR for dark theme jenkinsci/dark-theme-plugin#338

Thanks!

timja · 2023-03-12T18:12:10Z

There is an overlay (it darkens the page)

The overlay only works on safari, doesn't work on Firefox or Chrome

janfaracik · 2023-03-12T19:54:17Z

There is an overlay (it darkens the page)

The overlay only works on safari, doesn't work on Firefox or Chrome

Hadn't spotted that, thanks - opened a new dark theme PR to address janfaracik/dark-theme-plugin#44

timja · 2023-03-12T20:26:11Z

That’s been opened against your fork not the origin

janfaracik · 2023-03-12T20:51:57Z

That’s been opened against your fork not the origin

Whoops, opened it again jenkinsci/dark-theme-plugin#339

timja · 2023-03-12T22:16:48Z

war/src/main/webapp/scripts/prototype.js

-  remove: function(element) {
-    element = $(element);
-    element.parentNode.removeChild(element);
-    return element;
-  },


why was this removed? it's what is causing the ATH failures.

It was interfering with the removal of the modal (I'm assuming because the modal doesn't have a parent?

do we still need to remove this function now that you've changed the core components to not use it?
https://github.com/search?q=org%3Ajenkinsci+Element.remove%28+language%3AJavaScript&type=code&l=JavaScript&p=1

It looks like it should be okay to remove.

(Accidentally resolved this in case you missed it)

If I'm understanding correctly, we still need to remove the function as it interferes with removing the modal.

I tested locally and it seems to be working fine? not sure how it could be causing an issue if nothing is calling it.

With the remove() function restored I get the above error when closing the modal.

weird I get that error in Safari but not chrome...

Maybe it has a parent in chrome, I guess we already hit a browser difference with this functionality.

timja · 2023-03-13T22:20:49Z

ATH passed: jenkinsci/acceptance-test-harness#1062

NotMyFault

A very welcoming change :)

yaroslavafenkin

Tested manually on FF, Chrome and Safari, seems to behave as expected.
Security-wise looks good to me too.

timja · 2023-03-16T12:59:26Z

/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!

github-actions · 2023-03-17T09:37:09Z

Please take a moment and address the merge conflicts of your pull request. Thanks!

daniel-beck · 2023-03-17T17:04:16Z

core/src/main/resources/jenkins/model/Jenkins/legend.jelly

-    </l:main-panel>
-  </l:layout>
+<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
+  <h1 class="jenkins-modal__title">${%Icon legend}</h1>


Since this is no longer a standalone view, attempts to access the URL /legend (e.g. from links in outside documentation, or browser history) will result in the logging of the message

Mar 17, 2023 5:00:06 PM WARNING jenkins.security.stapler.StaplerFilteredActionListener onDispatchTrigger New Stapler dispatch rules result in the URL "/legend" no longer being allowed. If you consider it safe to use, add the following to the whitelist: "hudson.model.Hudson legend". Learn more: https://www.jenkins.io/redirect/stapler-facet-restrictions

This seems unnecessary. Perhaps rename the view to _legend so it's a simple 404?

Init

939e3c0

timja requested a review from a team March 12, 2023 16:26

timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted needs-security-review Awaiting review by a security team member labels Mar 12, 2023

timja requested a review from a team March 12, 2023 16:29

Close dialog if the user clicks outside of it

d82f112

janfaracik mentioned this pull request Mar 12, 2023

Add modal colors jenkinsci/dark-theme-plugin#338

Merged

Fix HTMLUnit 💩

acd9f8b

janfaracik added 2 commits March 12, 2023 19:27

Fix backdrop in Chrome/Safari, some tweaks

6bdd012

Update theme.less

89cf665

Fix build history legend link

d55034a

timja reviewed Mar 12, 2023

View reviewed changes

timja mentioned this pull request Mar 12, 2023

Small tidy up for the Tools page #7716

Merged

14 tasks

janfaracik and others added 2 commits March 12, 2023 23:15

Fix removal of Prototype remove

a7811a6

Fix linting

18f5d5f

NotMyFault added the web-ui The PR includes WebUI changes which may need special expertise label Mar 13, 2023

timja added a commit to jenkinsci/acceptance-test-harness that referenced this pull request Mar 13, 2023

Test jenkinsci/jenkins#7718

e2dbfee

timja mentioned this pull request Mar 13, 2023

Test https://github.com/jenkinsci/jenkins/pull/7718 jenkinsci/acceptance-test-harness#1062

Closed

janfaracik added 2 commits March 14, 2023 18:39

Merge branch 'master' into revamp-icon-legend-as-modal

c766f56

Merge branch 'master' into revamp-icon-legend-as-modal

7b8eaa3

NotMyFault approved these changes Mar 16, 2023

View reviewed changes

yaroslavafenkin approved these changes Mar 16, 2023

View reviewed changes

timja approved these changes Mar 16, 2023

View reviewed changes

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 16, 2023

timja added needs-security-review Awaiting review by a security team member 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 Mar 17, 2023

github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 17, 2023

Merge branch 'master' into revamp-icon-legend-as-modal

b355e69

github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 17, 2023

timja merged commit 38d3a27 into jenkinsci:master Mar 17, 2023

timja deleted the revamp-icon-legend-as-modal branch March 17, 2023 13:26

daniel-beck reviewed Mar 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp icon legend as a modal #7718

Revamp icon legend as a modal #7718

janfaracik commented Mar 12, 2023 •

edited

Loading

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

timja Mar 12, 2023

janfaracik Mar 12, 2023

timja Mar 13, 2023

timja Mar 14, 2023

janfaracik Mar 14, 2023

timja Mar 14, 2023

janfaracik Mar 15, 2023

timja Mar 15, 2023

timja commented Mar 13, 2023

NotMyFault left a comment

yaroslavafenkin left a comment

timja commented Mar 16, 2023

github-actions bot commented Mar 17, 2023

daniel-beck Mar 17, 2023

timja Mar 17, 2023

Revamp icon legend as a modal #7718

Revamp icon legend as a modal #7718

Conversation

janfaracik commented Mar 12, 2023 • edited Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

timja commented Mar 12, 2023

janfaracik commented Mar 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja commented Mar 13, 2023

NotMyFault left a comment

Choose a reason for hiding this comment

yaroslavafenkin left a comment

Choose a reason for hiding this comment

timja commented Mar 16, 2023

github-actions bot commented Mar 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janfaracik commented Mar 12, 2023 •

edited

Loading