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

Modernise UI #287

Merged
merged 14 commits into from
Mar 26, 2022
Merged

Modernise UI #287

merged 14 commits into from
Mar 26, 2022

Conversation

timja
Copy link
Member

@timja timja commented Mar 5, 2022

Requires jenkinsci/jenkins#6307

Principles applied:

  • All pages have a heading
  • Title matches the heading
  • All buttons say what they do instead of OK
  • All buttons on a new line not next to text
  • All primary buttons are styled appropriately
  • Switch to core controls where possible otherwise backport changes

TODO:

  • Get upstream PR merged

image

Manage Jenkins image
Add credential button spacing fixed image
Add credential dialog

image
image

Global credentials list image
New Credentials image
Update credentials image
Move credentials image
Delete credentials image
Add Domain

image

New domain image
View domain image
Update domain image
Delete domain image
Credentials parameter image

@NotMyFault
Copy link
Member

Not 100% sure about the whole security section having the same icon

image

Yeah, I'd preserve the lock for the "Configure Global Security" tab and use the key icon here.
Considering we have a key.svg in core, we could serve the icon from there too, thoughts?

@timja
Copy link
Member Author

timja commented Mar 5, 2022

yeah sure

@timja

This comment was marked as outdated.

@timja timja changed the title Modernise icons Modernise UI Mar 6, 2022
@timja timja mentioned this pull request Mar 6, 2022
6 tasks
@timja timja requested a review from a team March 6, 2022 11:48
@timja
Copy link
Member Author

timja commented Mar 6, 2022

Done all I plan to do, merging is blocked until jenkinsci/jenkins#6307 is released.

But I would appreciate reviews and feedback

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

  • The dropdown to add a new credentials domain on the "Store" column solely displays the chevron but not the context menu to perform the operation.

  • Now we have three icon sources in place, I'm wondering if it would be worth to replace/overwrite the table icons too 🤔
    | | |

}

.help-content::before {
Copy link
Member

Choose a reason for hiding this comment

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

What is that used for? Can't we rely on jenkins-help-button or l:helpIcon from core?

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s used when you add a credential parameter to a job.

credentials plugins has created a ‘simple help’ that can have inline text. Can maybe refactor it to use core in some way, this is just fixing it up to look right.

@timja
Copy link
Member Author

timja commented Mar 6, 2022

Sure can try that what are the symbols for each ?

@NotMyFault
Copy link
Member

Sure can try that what are the symbols for each ?

Ribbon for certificate.svg (overwriting in core), symbol-jenkins for system-scope.svg, symbol-key for credential.svg (overwriting in core) and I added symbol-id-card for the user pass one.

If you don't mind, you can simply apply my patches I used to test it with https://gist.github.com/NotMyFault/daeaeda507eb1c62dacc3bcfbbeb58c0

@timja
Copy link
Member Author

timja commented Mar 6, 2022

Looks good on the view credentials table:
image

Bit weird on the domain page because of the blue:

image

@NotMyFault
Copy link
Member

I guess the blue comes from core? Spots like dropdowns still use it, e.g.

@timja
Copy link
Member Author

timja commented Mar 6, 2022

Looks like because it's a link for some reason =/

pom.xml Outdated Show resolved Hide resolved
@@ -63,13 +64,13 @@
</f:dropdownListBlock>
</j:forEach>
</f:dropdownList>
<!--f:dropdownDescriptorSelector field="credentials" title="${%Kind}" lazy="it"/-->
<!--<f:dropdownDescriptorSelector field="credentials" title="${%Kind}" descriptors="${descriptors}"/> -->
Copy link
Member Author

Choose a reason for hiding this comment

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

After making these changes I think it works but didn't want to do it in the same PR

@timja
Copy link
Member Author

timja commented Mar 10, 2022

I've left out changes to the context menu for two reasons:

  1. I wasn't able to figure out how to apply a symbol to it short of setting the iconXml field which requires me to load a symbol in the code here which requires calling a restricted API (I disabled access restrictor and verified that works)
  2. Translations don't seem to apply to it, I tried using an Icon that has a translation but the context menu code path doesn't seem to go through that (I wasn't able to track down how it's actually built but I didn't spend too long tracing it).

FYI @janfaracik if you have any ideas but probably not critical for the initial PR

@timja timja marked this pull request as ready for review March 23, 2022 18:30
pom.xml Show resolved Hide resolved
@timja timja requested a review from a team March 25, 2022 16:07
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Looks amazing 🎉

@jglick
Copy link
Member

jglick commented Mar 26, 2022

@timja is this & #283 tested and ready to release?

@timja
Copy link
Member Author

timja commented Mar 26, 2022

This one is tested and ready for release.

I think #283 is fine, although it was fixed in core so the change isn't absolutely required, but I think there was something wrong with the SVG which @NotMyFault fixed so might be good to still ship it.

@timja timja mentioned this pull request Mar 26, 2022
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Seems OK though I do not follow details. @timja go ahead and merge if you like; seems you have permissions.

pom.xml Show resolved Hide resolved
@timja
Copy link
Member Author

timja commented Mar 26, 2022

Seems OK though I do not follow details. @timja go ahead and merge if you like; seems you have permissions.

I'm not a maintainer here, org owner gives me the green tick though.

@timja
Copy link
Member Author

timja commented Mar 26, 2022

Builds green now @jglick

@jglick jglick merged commit b4e24ac into jenkinsci:master Mar 26, 2022
@timja timja deleted the modern-icons branch March 26, 2022 17:56
@daniel-beck
Copy link
Member

May have caused JENKINS-68791

@@ -1075,7 +1075,7 @@ public String getDisplayName() {
*/
@Override
public String getIconClassName() {
return "icon-credentials-credentials";
return "symbol-key";
Copy link
Member

Choose a reason for hiding this comment

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

I think the wallet icon would have been a better choice for the reasons explained in JENKINS-69775.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants