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

[JENKINS-71578] allow making sidepanel sticky #8269

Merged
merged 9 commits into from Dec 19, 2023

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jul 14, 2023

Having a sticky sidepanel allows pages with only few links but longer content easier navigation.

Apply stickiness to PluginManager.

After:
image

See JENKINS-71578.

Testing done

Manually visit PluginManager
Sidepanel sticks when scrolling down so all links stay accessible all the time.
Configure a job -> sidepanel is still sticky

Proposed changelog entries

  • Allow to make side panel sticky

Proposed upgrade guidelines

N/A

Submitter checklist

Edit tasklist title
Beta Give feedback Tasklist Submitter checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. The Jira issue, if it exists, is well-described.
    Options
  2. 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.
    Options
  3. There is automated testing or an explanation as to why this change has no tests.
    Options
  4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
    Options
  5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
    Options
  6. 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).
    Options
  7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
    Options
  8. For new APIs and extension points, there is a link to at least one consumer.
    Options

Desired reviewers

@mention

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

Maintainer checklist

Edit tasklist title
Beta Give feedback Tasklist Maintainer checklist, more options

Delete tasklist

Delete tasklist block?
Are you sure? All relationships in this tasklist will be removed.
  1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
    Options
  2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
    Options
  3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
    Options
  4. Proper changelog labels are set so that the changelog can be generated automatically.
    Options
  5. 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).
    Options
  6. 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).
    Options

Having a sticky sidebar not only for app bars allows pages with only few
links but longer content easier navigation
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.

Needs to be accompanied by (plugin) developer guidance when to use this to prevent a mess of arbitrary choices.

Should not be used on extensible pages like for User, otherwise scrolling makes later entries inaccessible. (It's fairly unlikely to matter for User, but it's an extensible sidepanel nonetheless.)

@mawinter69
Copy link
Contributor Author

The non scrolling is already a problem now. Just go to configure job and make the browser window very small and the lower links are gone.
So maybe we should add some css to allow it to scroll if required

@daniel-beck
Copy link
Member

daniel-beck commented Jul 14, 2023

already a problem now

While small windows will hide sidepanel entries for config forms, it's the same 5 or so entries every time, and they're only for navigation inside the same page (something you can also accomplish by scrolling), not to go somewhere else. User and everything Actionable has an unbounded number of sidepanel entries, and those links go elsewhere with usually no alternative.

So maybe we should add some css to allow it to scroll if required

Would be an improvement in any case I think 👍

Which leaves when to use which. If scrolling sidepanels will be able to handle arbitrary numbers of entries nicely, and are better, why should this be optional, and opt-in?

when to use it and when not
@mawinter69
Copy link
Contributor Author

tested this on a job. When adding a overflow: auto and a scrollbar is shown, the widget fills up to the scrollbar and then when hovering the navigation arrows are missing. Can be overcome by adding padding to the sidepanel but this will increase the gap between side and main panel. And scrollbar-gutter is not supported on Safari 😞

@janfaracik
Copy link
Contributor

Would it be possible to move <div id="side-panel" class="app-page-body__sidebar"> from layout.jelly into side-panel.jelly and that way we could do away with the JS?

@mawinter69
Copy link
Contributor Author

mawinter69 commented Aug 17, 2023

Would it be possible to move <div id="side-panel" class="app-page-body__sidebar"> from layout.jelly into side-panel.jelly and that way we could do away with the JS?

done
A minor difference would be if someone creates a two column layout but is not including a sidepanel, then the div is not generated which might be expected when enabling YUI debug mode.

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

@timja timja added the developer Changes which impact plugin developers label Aug 19, 2023
@timja timja requested a review from daniel-beck August 19, 2023 17:06
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 21, 2023
@github-actions
Copy link

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 Aug 23, 2023
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Dec 17, 2023
@timja
Copy link
Member

timja commented Dec 17, 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 Dec 17, 2023
@NotMyFault NotMyFault merged commit a822935 into jenkinsci:master Dec 19, 2023
18 checks passed
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 web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
5 participants