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

Update appearance of keyboard shortcut tooltips #8435

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Aug 25, 2023

Small one in preparation for #7569. This PR adds a new component for showing keyboard shortcuts, it's currently used in the tooltip for search bars.

Screen.Recording.2023-08-25.at.21.29.41.mov

This will be used for the Command Palette button too, where hovering over the Search icon will list what shortcuts activate it.

Testing done

  • Shortcut and animation display as expected

Proposed changelog entries

  • N/A

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 the skip-changelog Should not be shown in the changelog label Aug 26, 2023
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.

works nicely

@timja timja requested a review from a team August 26, 2023 07:40
@timja timja added the needs-security-review Awaiting review by a security team member label Aug 26, 2023
@timja timja requested a review from a team August 26, 2023 07:40
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 from a security perspective!

@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 Aug 29, 2023
@timja
Copy link
Member

timja commented Aug 29, 2023

/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 Aug 29, 2023
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.

Is there a way to demonstrate the CMD/CTRL behavior? Right now it's unused code.

core/src/main/resources/lib/layout/keyboard-shortcut.jelly Outdated Show resolved Hide resolved
core/src/main/java/hudson/Functions.java Show resolved Hide resolved
</st:documentation>

<div class="jenkins-tooltip__keyboard-shortcut">
<j:if test="${attrs.prefix != null}">
Copy link
Member

Choose a reason for hiding this comment

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

It does not seem to be possible to define empty localizations. An attempt to do so will just use the default locale:

Screenshot 2023-08-29 at 14 29 18

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot - updated.

@daniel-beck daniel-beck removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Aug 29, 2023
@daniel-beck
Copy link
Member

Subjective design feedback: The animation doesn't look good at the actual size. I first thought it was a bug, in particular on the default (non-dark) theme.

weird.animation.firefox.mov

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.

Still doesn't work for me.
Screenshot 2023-12-06 at 14 49 09
(The build claims to be 2.434-SNAPSHOT so I'm not running the old one.)

Did you test this before resubmitting for review? If you did, what does the localized file look like?

@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Dec 16, 2023
@timja
Copy link
Member

timja commented Dec 16, 2023

Still doesn't work for me. Screenshot 2023-12-06 at 14 49 09 (The build claims to be 2.434-SNAPSHOT so I'm not running the old one.)

Did you test this before resubmitting for review? If you did, what does the localized file look like?

Works fine for me, there's a prefix and a suffix you need to update, I just fixed up a localisation (e.g. pl) as an example.

image

@timja timja added ath-successful This PR has successfully passed the full acceptance-test-harness suite and removed needs-ath-build Needs to run through the full acceptance-test-harness suite labels May 21, 2024
@timja
Copy link
Member

timja commented May 21, 2024

@daniel-beck I like your single string approach, you happy with this?

@timja
Copy link
Member

timja commented May 28, 2024

@daniel-beck you able to check this please?

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.

I didn't exactly provide a complete solution 😅

English localization is missing, so hovering just shows shortcut.

<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define">
<st:documentation>
<st:attribute name="message">
Optional message for the shortcut
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Optional message for the shortcut
Tooltip message for the shortcut. Use the placeholder <code>{0}</code> escaped (wrapped in <code>&quot;</code>). A minimal message consisting of only the shortcut would be <code>&quot;{0}&quot;</code>.

(Assuming <code> works here.)

Copy link
Member

Choose a reason for hiding this comment

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

it renders the code and &quot literally with the intellij plugin, unsure about stapler-maven-plugin itself, searching the code I couldn't see any handling of it though.

@janfaracik
Copy link
Contributor Author

@daniel-beck @timja Updated this - big thanks to both of you for the comments and assistance.

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.

LGTM - tested the latest changes and reviewed the code, much more in line with what was expected and cleaner, thanks. Apologies for the delay

@timja
Copy link
Member

timja commented Oct 6, 2024

Re-requested @NotMyFault review as the code has changed a fair bit since last approved.

@daniel-beck do you have an ETA / plan to review again?

Thanks

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 12, 2024
Copy link
Contributor

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

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 12, 2024
@timja
Copy link
Member

timja commented Oct 13, 2024

cc @NotMyFault ^^

@timja timja requested a review from a team October 15, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ath-successful This PR has successfully passed the full acceptance-test-harness suite security-approved @jenkinsci/core-security-review reviewed this PR for security issues skip-changelog Should not be shown in the changelog 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.

6 participants