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 60920] header breadcrumbs CSS update #4463

Merged

Conversation

fqueiruga
Copy link
Contributor

@fqueiruga fqueiruga commented Jan 30, 2020

See JENKINS-60920.

This PR includes the header and breadcrumb visual updates that came out of the work of the UX SIG.
Explanation for some technical decisions on the issue

NOTE: To enable/disable the change easily without restarting, you can use the /script URL (typically http://localhost:8080/script in local dev mode) and type hudson.Functions.UI_REFRESH=true to enable the new UI or hudson.Functions.UI_REFRESH=false to disable it.

Proposed changelog entries

  • Major change: visual revamp of the layout and icons of the header bar and breadcrumbs. Instances with plugins that depend on details of the Jenkins layout (e.g. Simple Theme Plugin) may experience UI/layout problems.
  • Major change: A new header color scheme can be enabled by setting the system property jenkins.ui.refresh to true.

Proposed upgrade guidelines

Instances with plugins that depend on details of the Jenkins layout (e.g. Simple Theme Plugin with custom CSS themes) may experience UI/layout problems. Minor patches to the themes may be required

Submitter checklist

  • 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). 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
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@uhafner
@timja

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 correct
  • 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 should exist and be labeled as lts-candidate

Screenshots

Classic UI on desktop
classic-ui-desktop

New UI on desktop
new-ui-desktop

Classic UI on mobile
classic-ui-mobile

New UI on mobile
new-ui-mobile

TODOs:

  • create a jelly helper for using the <svg> tag for sprite icons.
  • remove comments and port the layout.jelly changes to the html.jelly file.
  • migrate dimensions to rems. At least for the font-sizes.
  • fix for ie11
  • Optional: migrate the breadcrumbs separator and dropdown menu selector icons to svgs.

- Used the material icons svg sprites to deliver the icons
- Icons are rendered using a <svg> tag
- The new UI can be toggled with the -Dui.enableNewUi=true flag
- Rename the css class namespace from .main-header to .page-header
- Page header CSS moved into a modules folder and now included on the new-base-styles.css. This is done to avoid loading a separate file everytime.
- Extracted color variables to their own file
- The pageHeader template receives localization strings as props
- Missing translation for the admin monitor on the it, bg and pl locales
@batmat batmat added work-in-progress The PR is under active development, not ready to the final review web-ui The PR includes WebUI changes which may need special expertise labels Jan 30, 2020
@timja timja requested review from timja and uhafner January 30, 2020 16:16
@batmat batmat requested a review from varyvol January 30, 2020 17:27
@timja
Copy link
Member

timja commented Jan 30, 2020

Code looks fine to me, except the commented out code which is on the TODO list, will re-review when TODOs are completed

Tested locally with old and new UI on a mac 13".

@batmat
Copy link
Member

batmat commented Jan 30, 2020

@fqueiruga test failures look legit
image

@Wadeck Wadeck self-requested a review February 1, 2020 08:24
fqueiruga and others added 4 commits February 3, 2020 16:53
Co-Authored-By: Tim Jacomb <t.jacomb@kainos.com>
- Reverts breadcrumb link styling to use nested selectors instead of the .breadcrumbs__link class. The reason is that several plugins would break.
- Shifted margins so that breadcrumbs can be fullwidth in the absence of the auto refresh link
@fqueiruga
Copy link
Contributor Author

I have set up the feature flag but nothing else on the html.jelly file (https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/layout/html.jelly).
The reason is that I don't see any references to the header on that file. I'll do more changes if needed.

@uhafner
Copy link
Member

uhafner commented Feb 3, 2020

Thanks for providing the new styles as PR! It is much simpler to get an impression of the changes now...

Before I am looking into the PR just a few quick impressions: on a plain vanilla Jenkins everything looks really nice! One small thing: If I disable the new UI (using the property) I still get the new breadcrumbs, is that intended? Or should everything be restored to the original state?

Did you try to use it with a theme? I'm using http://afonsof.com/jenkins-material-theme/ on my machine but these kind of themes seem not to work anymore. Even if I disable the new UI with the property, the theme looks weird.

@imod
Copy link
Member

imod commented Feb 4, 2020

I use the same theme as @uhafner - but to be honest, I don't think we should take to much care - it does not seem to be maintained anymore

@uhafner
Copy link
Member

uhafner commented Feb 4, 2020

Hmm, the simple theme plugin has > 25000 installations. I'm not sure if it is only the material theme that will break, did you check one of the maintained themes like https://github.com/TobiX/jenkins-neo2-theme of @TobiX still works after the changes?

@timja
Copy link
Member

timja commented Feb 4, 2020

I thought it was expected that those plugins will have issues and will need to update? Possibly the need for those will go-away / decrease as the UI is improved / refreshed

@fqueiruga
Copy link
Contributor Author

Thanks Daniel, I'll surface these pieces of design feedback on the next SIG meeting. I find them interesting, especially the pluralization one.

@timja
Copy link
Member

timja commented Feb 18, 2020

@daniel-beck ux-sig is now on gitter https://gitter.im/jenkinsci/ux-sig

Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

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

Less files are pretty large, difficult to be sure everything is perfect, but it seems to be working fine. Just a few comments, no blockers.

core/src/main/java/hudson/Functions.java Outdated Show resolved Hide resolved
font-weight: bold;

display: inline-block;
display: inline-flex;
Copy link
Member

Choose a reason for hiding this comment

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

which is it, inline-block or inline-flex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It´s inline-flex but I left inline-block as a fallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my initial comment proposing to add a comment in the code seems even more accurate :)

color:#3465a4;
#breadcrumbs LI A:hover, #breadcrumbs LI A:focus,
.breadcrumbs__auto-refresh:hover, .breadcrumbs__auto-refresh:focus {
/* TODO: change hover state
Copy link
Member

Choose a reason for hiding this comment

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

Will it be part of a future work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, probably once we work on the contextual menu dropdown.

visibility: hidden;
cursor: pointer;
z-index: 2000;
background-color:transparent;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure those changes are required..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the indentation changes? The file indentation was a bit inconsistent as it had different indentation levels. I put all the lines on the same level.

<img src="${imagesURL}/jenkins-header-logo-new.svg"
alt="[${logoAlt}]"
class="page-header__brand-image" />
<span class="page-header__brand-name">Jenkins</span>
Copy link
Member

Choose a reason for hiding this comment

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

No, Jenkins is Jenkins in all languages.

Co-Authored-By: Adrien Lecharpentier <adrien.lecharpentier@gmail.com>
@timja timja closed this Feb 18, 2020
@timja timja reopened this Feb 18, 2020
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Taking the approvals, I suggest to merge it into 2.222 if there is no negative feedback by Thursday 4PM UTC

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback on-hold This pull request depends on another event/release, and it cannot be merged right now labels Feb 19, 2020
@timja timja removed the on-hold This pull request depends on another event/release, and it cannot be merged right now label Feb 20, 2020
@timja timja merged commit 673a18c into jenkinsci:master Feb 20, 2020
@daniel-beck
Copy link
Member

This PR introduces a minor regression on Firefox:

When jumping to an anchor on the page (e.g. through the dropdown menu for configuration forms), the breadcrumb bar doesn't move to the top of the page. Might be related to page zoom.

Expected result:

Screenshot

Actual result:

Screenshot

@oleg-nenashev oleg-nenashev added the upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed label Feb 27, 2020
@oleg-nenashev
Copy link
Member

@fqueiruga FYI I added a short upgrade guide entry to address the comments in https://groups.google.com/d/msg/jenkinsci-dev/VRxRCP_IKhQ/8Uw2SJqWAAAJ . It is based on your changelog entry text. Please feel free to extend it as needed

@CptPlastic
Copy link

Hmm, the simple theme plugin has > 25000 installations. I'm not sure if it is only the material theme that will break, did you check one of the maintained themes like https://github.com/TobiX/jenkins-neo2-theme of @TobiX still works after the changes?

I am in the same boat as many that use the material theme. When you try to enable this it just breaks the login page. Even if you remove the plugin some thing still lingers. Is there any guidance for people who do run into this because we are back to the classic theme and people notice. Is there a place I can go look to fix this for the folks that have Simple Theme installed?

@timja
Copy link
Member

timja commented Feb 27, 2020

Hmm, the simple theme plugin has > 25000 installations. I'm not sure if it is only the material theme that will break, did you check one of the maintained themes like TobiX/jenkins-neo2-theme of @TobiX still works after the changes?

I am in the same boat as many that use the material theme. When you try to enable this it just breaks the login page. Even if you remove the plugin some thing still lingers. Is there any guidance for people who do run into this because we are back to the classic theme and people notice. Is there a place I can go look to fix this for the folks that have Simple Theme installed?

each theme will need some updates done

i.e. material theme here:
https://github.com/afonsof/jenkins-material-theme

@CptPlastic
Copy link

CptPlastic commented Feb 27, 2020

Hmm, the simple theme plugin has > 25000 installations. I'm not sure if it is only the material theme that will break, did you check one of the maintained themes like TobiX/jenkins-neo2-theme of @TobiX still works after the changes?

I am in the same boat as many that use the material theme. When you try to enable this it just breaks the login page. Even if you remove the plugin some thing still lingers. Is there any guidance for people who do run into this because we are back to the classic theme and people notice. Is there a place I can go look to fix this for the folks that have Simple Theme installed?

each theme will need some updates done

i.e. material theme here:
https://github.com/afonsof/jenkins-material-theme

You see how that causes an issue though right? 25000 things have this and that is a pretty substantial impact. Even if they install that plugin and remove it the break still happens. The only way to unload it is to restart the service twice. I get that this plugin is not Jenkins's and some of the templates need updates but this breaks jenkins bottom line. There may be something jenkins can do to force the css thats for some reason making the login pages blank.

I think this is awesome and I would much rather have a nice UI that matched blue ocean and that you guys are giving some much needed attention to the UI I just wanted to make you aware that these things are happening.

@uhafner
Copy link
Member

uhafner commented Feb 27, 2020

i.e. material theme here:
https://github.com/afonsof/jenkins-material-theme

The material theme seems to be unmaintained currently. So if nobody steps in this theme will not be supported in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet