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

Modernize UI #364

Merged
merged 9 commits into from
Sep 16, 2022
Merged

Modernize UI #364

merged 9 commits into from
Sep 16, 2022

Conversation

timja
Copy link
Member

@timja timja commented Sep 4, 2022

  • Use context specific sidepanel
  • Remove sidepanel on main credentials page
  • Display store specific names rather than Jenkins for all of them on view credentials page
  • Remove norefresh from views
  • Maintain manage in url bar so that breadcrumbs work correctly

Note: screenshots taken on 2.366, on 2.361 the one-column layouts will be a bit wider and not look quite as good

Before After
View Credentials image image
View domain (one-column) image image
View domain (domain modifiable) image image
View store (domain not modifiable) image image
View store (domain modifiable) image image
Add domain image image

TODO:

@@ -58,6 +58,7 @@
import jenkins.model.ModelObjectWithContextMenu;
import jenkins.model.TransientActionFactory;
import org.acegisecurity.Authentication;
import org.apache.commons.lang.StringUtils;
Copy link
Member

Choose a reason for hiding this comment

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

Unused

@daniel-beck
Copy link
Member

Looks reasonable overall. Especially thanks for taking care of the confusing sidepanels :)

pom.xml Outdated Show resolved Hide resolved
* Resolves a display name from the Store
* @return the display name
*/
public String getDisplayName() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved code from CredentialsStoreAction

@timja timja marked this pull request as ready for review September 6, 2022 10:49
@timja timja requested review from a team September 6, 2022 10:50
@janfaracik
Copy link
Contributor

Looks good to me!


For this page, how would you feel about moving the 'Add domain' into the app bar and making it a primary button? (like the System Log page)

Screenshot 2022-09-06 at 20 07 02

@timja
Copy link
Member Author

timja commented Sep 6, 2022

Yep will try it, it was looking quite weird with a single item there

@timja
Copy link
Member Author

timja commented Sep 6, 2022

mind taking another look? I'll update screenshots later on

Edit: Screenshots updated

…lsStoreAction/DomainWrapper/index.jelly

Co-authored-by: Alexander Brandes <[email protected]>
@timja timja requested a review from jglick September 7, 2022 10:55
@timja timja mentioned this pull request Sep 8, 2022
@timja
Copy link
Member Author

timja commented Sep 13, 2022

@jenkinsci/credentials-plugin-developers possible for a review please?

@jtnord
Copy link
Member

jtnord commented Sep 14, 2022

Thanks TIm,

I tested this mostly on 2.361.1

when I go to "add a credential" I get a page (2 column) that seems out of place with the new design

Given I am in the "add credential" of a current domain" should the "confgure domain" delete/domain and "add credential" show up here? (even add credential which would just take me back to the same URL?)

image

--

I believe delete (or destructive action) buttons should still have a danger class (ie be red?). the screenshot in the description does - but it does not for me locally (chrome, windows)

domain overview page.

image

--

I am also a bit unsure how this fits into the "new way" as Jenkins still has side bars for "global tool config", "manage logs", "configure Global secruity" "manage nodes and cloud" etc. yet not for "manage credentials". this seems a bit jarring.

noticed things are being removed in the weeklies so this would be addressed by the time the next LTS rolls around.

--

When viewing Folder credentials there is no obvious way to guide you where to go. (yes the existing design is shitty) but there was a subtask under the credentials task which highlighted the "folder"

image

--

running a subset of ATHs to see if there are any impacts there

Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

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

something is definatly wrong with the delete button, but see more feedback above

(is red with 2.688 but not with the LTS).

@jtnord jtnord requested a review from a team September 14, 2022 16:42
@timja
Copy link
Member Author

timja commented Sep 14, 2022

all the feedback should be addressed

the class was renamed in 2.364 I could add both, but some of the pages look a bit funny on a one-column layout without other changes in 2.364 so I think better to just raise the baseline to that.

When viewing Folder credentials there is no obvious way to guide you where to go. (yes the existing design is shitty) but there was a subtask under the credentials task which highlighted the "folder"

I have potential future plans to remove the stores from that page, add a search bar and create a new create credential experience where you can choose the store / type / context in a nicer page.

but I think out of scope of this PR and this should be good enough for now?

@timja timja requested a review from jtnord September 14, 2022 19:24
@jtnord
Copy link
Member

jtnord commented Sep 15, 2022

the class was renamed in 2.364 I could add both, but some of the pages look a bit funny on a one-column layout without other changes in 2.364 so I think better to just raise the baseline to that.

I'm a bit hesitant on bumping the baseline this early in the LTS cycle. (There's some fixes still needed to be written for the icon change) and I would like to avoid branching if possible.

Additionally I thought the pages whilst not perfect in the LTS where still an improvement over the previous code.

Would you be against dropping the version requirement to the LTS and adding the old class name in addition?

@timja
Copy link
Member Author

timja commented Sep 15, 2022

Done

@jtnord jtnord merged commit c0a1f19 into jenkinsci:master Sep 16, 2022
@jtnord
Copy link
Member

jtnord commented Sep 16, 2022

Thanks @timja

@timja timja deleted the rework branch September 16, 2022 16:19
@ite-klass
Copy link
Contributor

A more descriptive PR title would have been helpful for the release changelog

@timja
Copy link
Member Author

timja commented Sep 21, 2022

A more descriptive PR title would have been helpful for the release changelog

see the PR description, or feel free to suggest a new one.

@ite-klass
Copy link
Contributor

Modernize UI would clarify it is about user interface / presentation changes

@timja timja changed the title Modernise Modernize UI Sep 21, 2022
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.

6 participants