-
Notifications
You must be signed in to change notification settings - Fork 250
Use card layout for credentials tables #984
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
Conversation
| value="${c.description==null?'':app.markupFormatter.translate(c.description)}"/> | ||
|
|
||
| <div class="credentials-wrapper"> | ||
| <div class="credentials-card"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with the jenkins-card but its title can't take html / an icon and I don't think its flexible enough for this sort of thing.
I used the exact same layout and css as the API tokens card but didn't want to reuse the classes as they aren't part of the API.
cc @mawinter69
|
Looking good! Just a few 8am ideas:
... could I get commit permissions to your fork 🙏 |
Sure sounds good
Definitely delete and move could be dialogs, they are very simple pages.
Makes sense, (assuming not expensive to get)
Done, go to https://github.com/timja/credentials-plugin/invitations |
|
@janfaracik thanks for the help! I've fixed up a few permissions issues where it wasn't quite right for credentials providers that you can't update from directly inside Jenkins (I tested with the Azure Key Vault plugin). I've also added empty states for when there's no credentials. Not the biggest fan of the root tables one as I haven't added a call to action due to there being multiple stores and how you can disable them etc I’d like to add this in a follow up I've created a PR to ATH as well. Anything else before this is ready do you think? |
Looks good to me 🚀 |
Can anybody think of a good ionicon to replace this? (https://ionic.io/ionicons) I can always make one if needed. |
|
@jglick possible to take a look at this please? |
jglick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A display nit. Otherwise seems OK to me from very brief local testing, but I lack any context for why you are redesigning GUI here. If you plan to work on a lot of stuff like this you should just request write access to the repo.
| class="jenkins-badge"> | ||
| <j:choose> | ||
| <j:when test="${domain.global}"> | ||
| ${%global} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably requires jenkinsci/jenkins@17abe4f#diff-39fd25ee020b2666775defabaaa2349f0cf5d77b65be30ceb02119e60f307be2
(2.535)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would file separately but Windows tests in this repo are rediculously slow
|
@timja this removed the model links from the "Stores" section of the overview page so you can no longer quickly add credentials or domains from the overview page, now you need to go to another page and then click introducing an uneeded step? This to me is a UX regression for power users, and I hope this was unintentional and can be restored? |
New credentials is going to be added this week in #992 with a better experience. Is new domains really needed? Are they used much at all? |
❤️
I personally rarely use them, and there are none that I see in CloudBees' instances. But that is possibly as they are currently hard to understand (there is no reason why a github PAT should not be in a domain for github.com - I think its partially the UX to create them is messy and does not guide people to use them, and partially that they do not restrict credentials in them to being used on those domains, but rather restricts requests with a domain to only those credentials). TBH as they are so rarely used we could possibly change that behaviour to make them actually more useful! |
|
The new credentials is ready now if you want to take a look. I’m working on ATH and will run it through pct although not expecting anything there |
|
IMO the concept of credentials domains was never needed and could be deprecated. |
| * @return the icon class name for the store. | ||
| * @since TODO | ||
| */ | ||
| public final String getIconClassName() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broke several CredentialsStore implementations in CloudBees CI which were implementing IconSpec (and thus defining their own getIconClassName methods). Is there a recommended migration?







Goal of this is to reduce duplication of information in e.g.:

Whilst making it easier to read and understand.
Also should stop credentials with longer names of descriptions from overflowing and breaking out of the page.
Root action before
Root action after
The above highlights that the display names are not the best for all credential types. I think its optimised for the credentials picker dropdown, but not great for the credentials list, I'd like to look at this if I have time but not for this PR
Domain
Empty state domain modifiable
Empty state domain not modifiable
Empty state root action
Testing done
Added a few credentials of differing types.
Added ones without description
Tested with read only stores (Azure key vault)
Ath passed as at b8944f9 in jenkinsci/acceptance-test-harness#2565
cc @janfaracik
Submitter checklist