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

Bring native support for side panel app bars, sticky side bar and changing app bar heading level #7051

Merged
merged 25 commits into from
Oct 13, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Sep 1, 2022

(Apologies for long title - I truly couldn't think of anything shorter).

This PR brings native support for some of the app bar changes introduced in #6485, rather than the 'one-off' implementations in that PR.

What's changed

  • App bars can now be applied to side panels and main panels
  • Side panels can now be sticky (with a new class)
  • You can now set the heading level for an app bar (e.g. change it to h2 from h1)

As a result of this the implementation of the app bar has changed, you now need to include it in either side panel or main panel (this may break a couple of plugins - I'll fix them).

This is the core change that jenkinsci/design-library-plugin#90 is waiting on, it essentially defines some clearer guidelines for when to use certain page layouts.

Downstream pull requests that need merging after this is released:

Proposed changelog entries

  • Developer: Add support for the app-bar component in the sidepanel.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, 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 directives (see documentation on jenkins.io).
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least 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 does not block the change
  • Changelog entries in the PR 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, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

@NotMyFault NotMyFault requested a review from a team September 2, 2022 09:21
@NotMyFault NotMyFault added the developer Changes which impact plugin developers label Sep 2, 2022
@daniel-beck daniel-beck self-requested a review September 2, 2022 12:09
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Sep 5, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 6, 2022
@github-actions
Copy link

github-actions bot commented Sep 6, 2022

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 Sep 6, 2022
@timja
Copy link
Member

timja commented Sep 6, 2022

This looks great but plugins using it already need adapting

Example of what credentials plugin looks like atm:

image

@janfaracik
Copy link
Contributor Author

This looks great but plugins using it already need adapting

  • job-config-history-plugin
  • credentials-plugin
  • design-library-plugin

Example of what credentials plugin looks like atm:

image

Wondering if there's a workaround for this as if we move the app-bars into main-panel for plugins it'll only work for this release, older versions of Jenkins will be missing titles.

@timja
Copy link
Member

timja commented Sep 12, 2022

/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 Sep 12, 2022
@daniel-beck
Copy link
Member

I don't understand why this reuses the app-bar element in the sidepanel. Do we expect sidepanel content to also have associated buttons etc?

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

daniel-beck commented Sep 13, 2022

"About Jenkins" is broken compared to 2.367:

Screenshot 2022-09-13 at 09 52 31

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.

Per above

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 14, 2022
@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 Sep 14, 2022
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

I like these new ideas! I would recommend, that those views of this PR that now use these new pattern should be completely adapted before we merge this PR. Otherwise it is totally unclear for plugin developers what to do and what to avoid. These core views should be the template for everybody else on how to move forward!

Some elements that are not adapted:

  • The side panel action name should be the same as the app bar title ("Updates", "Available Plugins" vs. "Plugins" or "Log Levels" vs. "Logger Configuration"). Maybe this can be done in the framework so I do not need to repeat
  • The app bar does only sometimes contain the buttons (Add new logger is correctly in the app bar, Save button of Logger Configuration is at the bottom)

Some bugs:

  • Log Recorders -> All Jenkins Logs break the sidebar completely (there is no sidebar anymore)
  • Global Tool Configuration and Configure System (and the other elements of the manage screen) have no sidebar anymore

Missing feature:
One thing that I am totally missing: the new sidebar elements should not scroll if I scroll down the page. These parts should use separate viewpoints. It does not make sense to scroll the new sidebar anymore, it is up to the developer to restrict this size.

@daniel-beck
Copy link
Member

the new sidebar elements should not scroll if I scroll down the page. These parts should use separate viewpoints. It does not make sense to scroll the new sidebar anymore, it is up to the developer to restrict this size.

@uhafner Could you clarify whether this only applies to sidepanels with app-bar, i.e., would there be some (most) sidepanels that scroll, and some that don't?

@uhafner
Copy link
Member

uhafner commented Sep 21, 2022

the new sidebar elements should not scroll if I scroll down the page. These parts should use separate viewpoints. It does not make sense to scroll the new sidebar anymore, it is up to the developer to restrict this size.

@uhafner Could you clarify whether this only applies to sidepanels with app-bar, i.e., would there be some (most) sidepanels that scroll, and some that don't?

If possible, all side panels should not scroll with the content. (I'm not sure if this works for the Dashboard view with the side panel widgets). I see no reason why the "links-only" navigation bars should scroll with the content.

@janfaracik
Copy link
Contributor Author

I don't understand why this reuses the app-bar element in the sidepanel. Do we expect sidepanel content to also have associated buttons etc?

I don't see why not really, there isn't a use case yet on Jenkins but it wouldn't hurt to share the same component to make updating both easier in the future.


  • The side panel action name should be the same as the app bar title ("Updates", "Available Plugins" vs. "Plugins" or "Log Levels" vs. "Logger Configuration"). Maybe this can be done in the framework so I do not need to repeat

That's a very good idea 👍 Not too sure what the best way of doing that right now is, might require a new component I'm not too sure.

  • The app bar does only sometimes contain the buttons (Add new logger is correctly in the app bar, Save button of Logger Configuration is at the bottom)

Not too sure on the best spot for 'Save'/'Apply' buttons right now. Thinking that they make more sense on the bottom as you interact with form elements before interacting with the 'Save' buttons.

Some bugs:

  • Log Recorders -> All Jenkins Logs break the sidebar completely (there is no sidebar anymore)
  • Global Tool Configuration and Configure System (and the other elements of the manage screen) have no sidebar anymore

Sidepanels have been removed from those pages as part of #6907 :) I've updated the 'All Jenkins Logs' pages to use a one column layout now so it should look better.

Missing feature: One thing that I am totally missing: the new sidebar elements should not scroll if I scroll down the page. These parts should use separate viewpoints. It does not make sense to scroll the new sidebar anymore, it is up to the developer to restrict this size.

If I'm understanding correctly, you'd want the sidebar to scroll separately to the main page contents/stick to the top if too short? If so I agree, need to sort out which pages should have side panels first before making a change like that though.

@timja timja requested a review from uhafner October 1, 2022 15:18
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, looks good to me

@NotMyFault
Copy link
Member

Thanks for addressing and clarifying the outstanding feedback!


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@NotMyFault NotMyFault added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Oct 3, 2022
@timja timja merged commit 7dd6883 into jenkinsci:master Oct 13, 2022
@timja timja deleted the revamp-app-bar branch October 13, 2022 19:47
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 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
6 participants