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-50061] Re-styled header #3285

Closed
wants to merge 20 commits into from
Closed

Conversation

recena
Copy link
Contributor

@recena recena commented Feb 10, 2018

See JENKINS-50061.

  • Update JTH to use a more recent release of HtmlUnit JENKINS-49491
  • Get a success execution of Functional tests for Jenkins core
  • Get a success execution of Acceptance test cases

Screenshots (before)

Firefox on Mac

screenshot-ci jenkins io-2018-03-04-20-21-16

Screenshots (after)

Chrome on Mac

menubar_and_jenkins_and_re-styled_header_by_recena_ pull_request__3285 _jenkinsci_jenkins

menubar_and_dashboard__jenkins__and_layout-common_css_-jenkins-___development_projects_jenkins

Firefox on Mac

screenshot-localhost 8081-2018-03-04-16-37-13

screenshot-localhost 8081-2018-03-04-16-39-16

Proposed changelog entries

  • Entry 1: Issue, Human-readable Text
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

Desired reviewers

@jenkinsci/code-reviewers

<j:set var="searchURL" value="${h.searchURL}"/>
<form action="${searchURL}" method="get" class="no-json" name="search">
<input name="q" placeholder="${%Search}" id="search-box" class="has-default-text" value="${request.getParameter('q')}" />
<!-- <div id="search-box-completion" /> -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is still in progress. I would like to make a proposal for removing this behavior.

@oleg-nenashev oleg-nenashev added the work-in-progress The PR is under active development, not ready to the final review label Feb 11, 2018
@jglick jglick changed the title [WiP] Re-styled header Re-styled header Feb 12, 2018
@Wadeck
Copy link
Contributor

Wadeck commented Feb 15, 2018

Here are screenshots under Windows

Chrome

chrome_none
No security

chrome_anon
Anonymous user

chrome_admin
Logged in as admin

Firefox

firefox_none
No security

firefox_anon
Anonymous user

firefox_admin
Logged in as admin

Edge

edge_none
No security

edge_anon
Anonymous user

edge_admin
Logged in as admin

Internet Explorer 11

ie-11_none
No security

ie-11_anon
Anonymous user

ie-11_admin
Logged in as admin

Feedback:

  • the new style is nice
  • the distinction between sign-in icon and sign-up label is quite confusing, seems there is only one possible action
  • the vertical align between icon + label is not consistent, but I know that's an NP-hard problem in browser compability science.
  • ⚠️ under IE 11 the butler is broken (normally displayed in previous version)

@recena
Copy link
Contributor Author

recena commented Feb 16, 2018

Awesome! I'm going to continue this weekend.

@daniel-beck
Copy link
Member

The position of the admin monitor popup, as well as the 'arrow' between popup and button, seem off. During the animation, the are misaligned with each other as well.

@daniel-beck
Copy link
Member

Also seems to remove the recently implemented admin monitor styling in the popup.

@recena
Copy link
Contributor Author

recena commented Feb 18, 2018

@daniel-beck

Also seems to remove the recently implemented admin monitor styling in the popup.

Could you provide a screenshot? I'm not aware about it.

@daniel-beck
Copy link
Member

PR build:

screen shot

2.107:

screen shot

@recena
Copy link
Contributor Author

recena commented Feb 19, 2018

@daniel-beck In 2.105

screenshot-localhost 8080-2018-02-19-23-49-47

Some alert box needs to be adapted in the main page.

@daniel-beck
Copy link
Member

Looks like there's a systematic problem with the warning style. Or both info and danger. @recena Could you look into that?

Manage Jenkins page

screen shot

Popup

screen shot

@recena
Copy link
Contributor Author

recena commented Feb 20, 2018

@daniel-beck Yes, I'm on it.

@recena
Copy link
Contributor Author

recena commented Mar 4, 2018

@Wadeck IMHO the current implementation is a bit more robust regarding to the alignment.

@@ -231,7 +231,7 @@
ListView view = listView("v");
view.description = "one";
WebClient wc = j.createWebClient();
String xml = wc.goToXml("view/v/config.xml").getContent();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated.

@recena
Copy link
Contributor Author

recena commented Mar 4, 2018

This piece of code is very fragile:

<script><![CDATA[
var okButton = makeButton($('ok'),null);

function updateOk(form) {
  function state() {
    if($('name').value.length==0)   return true;
    var radio = form.elements['mode'];
    if (radio.length==2)  return false; // this means we only have dummy checkboxes
    for(i=0;i<radio.length;i++)
      if(radio[i].checked)
        return false;
    return true;
  }

  okButton.set('disabled',state(),false);
}
updateOk(okButton.getForm());
]]></script>

If you use something like <img id="name" ..... /> many forms will stop of working.

@Wadeck
Copy link
Contributor

Wadeck commented Mar 5, 2018

@recena after update the look & feel is even nicer! go on!

Screenshots Windows

Chrome without any security

chrome-none

Chrome with security enabled
chrome-login

Chrome logged in

chrome-logged


⚠️ Little proposal: increase the space between the user context menu arrow and the log out icon since at the moment there is no pixel of space between them, it's "easy" to misclick and so to disconnect instead of going to configure page.

🐛 Bugs: signup

The signup button is lost in action. ✝️
chrome-signup-missing

Ho no, it was just hidden... it is a bit shy...
chrome-signup-found


Screenshots Other

Firefox
ff-none

Edge
edge-none

Internet Explorer 11
ie-none


PS: The graphical bug I saw on IE is no more there, good job 👍 .

@Wadeck
Copy link
Contributor

Wadeck commented Mar 5, 2018

For the behavior close to the context menu:

context-logout

  1. The arrow from the context menu stays during some seconds after we are out of its area, letting the user thinks it's still active.
  2. As the logout icon clickable area is too (pixel perfectly) close, a miss-click is too easy. In terms of UX it's not good I think.

@recena
Copy link
Contributor Author

recena commented Mar 5, 2018

@Wadeck What do you think?

screenshot-localhost 8081-2018-03-05-22-26-37

@Wadeck
Copy link
Contributor

Wadeck commented Mar 6, 2018

@recena your screenshot looks really good. I tested with other situation, other problems arise.

  1. If you have warnings, they are not glued with the search bar, an inexperimented admin could imagine it's a feature linked with the search input.

recena-3-glued-warning

  1. One logged in, as you put some space between the arrow (that is often invisible) and the log out icon, the username is not centered.

screenshot_2018-03-06_105009_001 - copie

As a proposal:
recena-3-proposal

To achieve this one:

  1. add margin-right: 6px to #visible-am-container (for warnings)
  2. add new rule #header ul#toolbar li.username a with padding-left:16px inside (for centered username without touching the breadcrumb or other link in header)

@recena
Copy link
Contributor Author

recena commented Mar 6, 2018

@Wadeck Great feedback. I'll address it.

@recena
Copy link
Contributor Author

recena commented Mar 8, 2018

@Wadeck Addressed. What do you think?

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

@recena thank you for the modification, that's great :)
👍

@recena recena changed the title Re-styled header [JENKINS-50061] Re-styled header Mar 10, 2018
@daniel-beck
Copy link
Member

The smaller resolution of the title text image makes it look much worse when the web page is zoomed in, and, I assume, on Retina Macs. What motivates changing this resource file?

@recena
Copy link
Contributor Author

recena commented Mar 11, 2018

@daniel-beck Could you provide a screenshot?

@recena
Copy link
Contributor Author

recena commented Mar 11, 2018

@daniel-beck General speaking, I'm working, through several PRs, on replacing several old-resources by new ones.

  • Re-organize style.css
  • Replace responsive-grid.css
  • Replace color.css

@recena
Copy link
Contributor Author

recena commented Dec 9, 2018

I think I would have to finish this proposal...

@oleg-nenashev oleg-nenashev added web-ui The PR includes WebUI changes which may need special expertise unresolved-merge-conflict There is a merge conflict with the target branch. labels Jan 31, 2019
@oleg-nenashev
Copy link
Member

It would be nice to get it landed, but it requires a merge conflict fix

@daniel-beck
Copy link
Member

@recena Could you clarify whether you're still interested in getting this change merged?

@batmat
Copy link
Member

batmat commented Apr 11, 2019

We will likely close this on next pass in the next days if nothing is done in the meantime.

If so, please feel free anytime to get back to this and tell us so. We'll be happy to reopen as/if needed.

Thanks!

@batmat batmat added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Apr 11, 2019
@recena
Copy link
Contributor Author

recena commented Apr 20, 2019

Ok, I will file a new PR if needed.

@batmat
Copy link
Member

batmat commented Apr 20, 2019

Great! Thanks for the feedback. Closing this one then. Thanks again

@batmat batmat closed this Apr 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it unresolved-merge-conflict There is a merge conflict with the target branch. web-ui The PR includes WebUI changes which may need special expertise work-in-progress The PR is under active development, not ready to the final review
Projects
None yet
5 participants