8000 Keycloak account console PatternFly design system alignment by tyandor · Pull Request #10748 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Keycloak account console PatternFly design system alignment #10748

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 6, 2022
Merged

Keycloak account console PatternFly design system alignment #10748

merged 1 commit into from
Apr 6, 2022

Conversation

tyandor
Copy link
Contributor
@tyandor tyandor commented Mar 14, 2022 •
edited by stianst
Loading

This work updates the Keycloak Account Console to the latest version of the PatternFly design system (release 2021.15) and follows the latest PatternFly design guidelines. This also brings the account console into visual / design alignment with the latest updates to the admin console.

See the design proposal for more details.

Closes #11103

@tyandor
Copy link
Contributor Author
tyandor commented Mar 14, 2022

@ssilvert @jonkoops @edewit @vmuzikar apologies for the inconvenience, but I had to recreate this. The commits are now accurate to just this PR and build should be working.

@jonkoops jonkoops self-requested a review March 14, 2022 21:17
@edewit
Copy link
Contributor
edewit commented Mar 15, 2022

so I also need to recreate my review... or can you see the things that I put on the other PR?

@edewit
Copy link
Contributor
edewit commented Mar 15, 2022

on the card page the "hamburger" icon and the signout button are both visible:
image

@tyandor
Copy link
Contributor Author
tyandor commented Mar 15, 2022

so I also need to recreate my review... or can you see the things that I put on the other PR?

@edewit I can still see the closed PR

@edewit
Copy link
Contributor
edewit commented Mar 15, 2022

@tyandor okay recreated the review comments anyway

edewit
edewit requested changes Mar 15, 2022
View reviewed changes
jonkoops
jonkoops requested changes Mar 15, 2022
View reviewed changes
stianst
stianst reviewed Mar 18, 2022
View reviewed changes
Copy link
Contributor
@stianst stianst left a comment

Choose a reason for hiding this comment

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

One thing to bear in mind is that this will be a significant change for anyone that customises the account console, so we will need an entry in the migration guide.

@stianst stianst requested a review from ssilvert March 18, 2022 09:27
ssilvert
ssilvert requested changes Mar 21, 2022
View reviewed changes
Copy link
Contributor
@ssilvert ssilvert left a comment

Choose a reason for hiding this comment

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

I won't repeat the thorough code review done by Erik and Jon, but I have manually tested this and overall it looks good.

The only issue I found was the same thing Erik found where he says, "on the card page the "hamburger" icon and the signout button are both visible". I think he is actually talking about the kebab menu. The kebab should not be shown in large screen mode. The way this works is that when screen is large, we show "Back to " and "Sign out" button. When screen is small we hide those and show kebab menu instead.

Alternatively, I'd be ok if we just got rid of the kebab menu altogether. Right now I don't see it in the application, but only see it on the home page.

In the application "Back to " appears left justified and butts up against the Keycloak logo when screen is small. So if we do get rid of the kebab menu, that should be addressed.

I have manually tested everything except "Resources". @tyandor have you tested this section? If not, one of use needs to go through the trouble of testing it.

As for automated testing, it's such a pain to run our Arquillian tests for this, I'm leaning against making you do it. They will probably fail and they need to be rewritten in Cypress anyway.

So as I see it, to merge we need:

  1. Address code changes from Erik and Jon.
  2. Fix kebab issue
  3. Update migration guide
  4. Verify that Resources section works. If nobody has done that yet, I can take care of it.

@tyandor
Copy link
Contributor Author
tyandor commented Mar 21, 2022

@ssilvert sounds good! I'm working through just a couple things from earlier feedback, and will take another shot at setting up a resource server today. We ran into issues with that previously following the docs and book, so might need you to validate.

@tyandor
Copy link
Contributor Author
tyandor commented Mar 23, 2022

@ssilvert @jonkoops @edewit I've completed all the fixes and updates if you want to verify. Assuming the process is for y'all to validate issues are resolved, so I just added comments.

Stan, are you able to validate Resources? I still haven't been able to get that set up. And any guidance on what is involved in updating the migration guide would be helpful.

edewit
edewit previously approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor
@edewit edewit left a comment

Choose a reason for hiding this comment

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

👍

@ssilvert
Copy link
Contributor
ssilvert commented Mar 27, 2022 •
edited
Loading

Stan, are you able to validate Resources? I still haven't been able to get that set up. And any guidance on what is involved in updating the migration guide would be helpful.

Getting resource stuff set up is a beast. I finally got the photoz example working well enough to test account console and it looks good. I also verified the issue with the kabab is resolved. So I'm happy with the code at this point.

As for the migration guide, I think @stianst was referring to the Upgrading Guide. The source is here: https://github.com/keycloak/keycloak-documentation/tree/main/upgrading

We should make developers aware that the PatternFly version was significantly updated and any customization they made to develop their own account UI might not be compatible. Perhaps a link to any PatternFly documents or insights you relied on to do this work would be helpful to those developers.

ssilvert
ssilvert previously approved these changes Mar 27, 2022
View reviewed changes
@tyandor
Copy link
Contributor Author
tyandor commented Mar 28, 2022

Thanks @ssilvert! We'll probably need someone from docs to help with the update there. At this point is there anything preventing this from being merged?

@ssilvert
Copy link
Contributor

Thanks @ssilvert! We'll probably need someone from docs to help with the update there. At this point is there anything preventing this from being merged?

@tyandor I think you will need to update the document yourself. Our docs guy can review if you want.

@stianst I think the code is ready, but I'll leave it up to you as to whether we can merge before the migration guide is updated. Since it's close to release time, I'm not sure if you want to hold off or not.

jonkoops
jonkoops previously approved these changes Mar 30, 2022
View reviewed changes
tyandor added a commit to tyandor/keycloak-documentation that referenced this pull request Mar 30, 2022
@tyandor
Update changes-18_0_0.adoc
65a41e6 
Adding an entry for Account console Patternfly upgrade. PR - keycloak/keycloak#10748
@tyandor tyandor mentioned this pull request Mar 30, 2022
Merged
@tyandor
Copy link
Contributor Author
tyandor commented Mar 30, 2022

@ssilvert @stianst here is the PR for docs: keycloak/keycloak-documentation#1452

@stianst stianst force-pushed the account-console-patternfly-upgrade branch from 148fba8 to a4b7c5b Compare April 4, 2022 06:09
@stianst
Copy link
Contributor
stianst commented Apr 4, 2022 •
edited
Loading

Would be good to get commits squashed for this one, and there's also a large number of test failures that needs to be resolved before we can merge.

Do we have a GitHub issue for this one?

If we want this to be included in Keycloak 18 we need the above sorted out asap.

@stianst stianst mentioned this pull request Apr 4, 2022
Closed
29 tasks
@hmlnarik hmlnarik dismissed stale reviews from jonkoops, ssilvert, and edewit via a4b7c5b April 4, 2022 13:52
@tyandor tyandor force-pushed the account-console-patternfly-upgrade branch from a4b7c5b to 1fbc88c Compare April 4, 2022 17:42
@tyandor tyandor force-pushed the account-console-patternfly-upgrade branch 3 times, most recently from 4f28ea6 to 0c6d9dc Compare April 5, 2022 02:59
@tyandor tyandor mentioned this pull request Apr 5, 2022
Closed
@tyandor
Copy link
Contributor Author
tyandor commented Apr 5, 2022

Would be good to get commits squashed for this one, and there's also a large number of test failures that needs to be resolved before we can merge.

Do we have a GitHub issue for this one?

If we want this to be included in Keycloak 18 we need the above sorted out asap.

@stianst the commits have been squashed and the test failures should be resolved (all succeeded for me locally).

Here is the issue.

@stianst stianst added this to the 18.0.0 milestone Apr 5, 2022
@tyandor
Updates patternfly libs and fixes breaking changes
2d931ae 
adding nvmrc

CIAM-1048 Device Activity screen PF updates

CIAM-1046: Personal Info sub-header update

Updates SigningInPage to use EmptyState component when there are no credentials.

rearanged some components used in signing in page

Displays ApplicationPage content in description list.

Updates refresh link on ContentPage, updates Resources screen.

CIAM-1049 Linked Accounts screen PF updates

CIAM-1043-General upstream updates

Updates AccountPage to display form errors.

fix: display Set up Authenticator Application link on large viewport

fix(page structure): rearranges page sections

CIAM-1254/Personal info PF4 updates & Sidebar text updates

updating layouts

updating layout on Signing in and Linked acounts

adding patternfly-additions

adding patternfly-addons styles

Updates Application page based on designs feedback.

moving page description

Updates status label on Applications page to be capitalized.

Updates the copy-fonts script for keycloak.v2 to copy all font directories instead of one.

update Personal info screen - set max width of 600px for form input fields

update Personal info - remove required indicator from input fields

General updates (#2)

* removed the extra lines being shown

* tweaked general spacing

* general alignment and spacer application

* refactor to get proper alignments without css globals

* forgot to add the conditional on displaying the set up buttons

* try and adjust the alignments

Co-authored-by: zwitter <zwitter@redhat.com>

resolve merge conflicts

Device activity updates (#4)

* update text to sentence case

* update device info columns to be dynamic across various viewport sizes

* update signed in device layout

* update based on feedback

Co-authored-by: Jon Szeto <jszeto@redhat.com>

Linked accounts update (#3)

* linked accounts screen - updated icons & Linked/Unlinked Login Providers layout & update text to sentence case

Co-authored-by: Jon Szeto <jszeto@redhat.com>

fixing ts errors

cleaning up fonts and messages

final review updates

message update for Back to admin console link

fixing capitalization on 2fa

updating landing page welcome message

fix: reposition Back to... link

adjusting size for confirm modal

updating spacing and alignment issues

updating resources page

removing unused header class

fixes ts issues and updates node version to match the themes install

npm updates

fixing pf addons

adding chokidar to get babel:watch working

fixing issues from pull request feedback

fixing tests

fixes signingin page test

fixing tests
@tyandor tyandor force-pushed the account-console-patternfly-upgrade branch from 0c6d9dc to 2d931ae Compare April 5, 2022 20:22
jonkoops
jonkoops requested changes Apr 6, 2022
View reviewed changes
...ources/theme/keycloak.v2/account/src/app/content/linked-accounts-page/LinkedAccountsPage.tsx

return (<CubeIcon id={`${account.providerAlias}-idp-icon-default`} size='xl'/>);
const socialIconId = `${account.providerAlias}-idp-icon-social`;
console.log(account);
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray console statement.

Suggested change
console.log(account);

...ources/theme/keycloak.v2/account/src/app/content/linked-accounts-page/LinkedAccountsPage.tsx
return (<CubeIcon id={`${account.providerAlias}-idp-icon-default`} size='xl'/>);
const socialIconId = `${account.providerAlias}-idp-icon-social`;
console.log(account);
switch (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code cannot be right, none of the cases besides the default will ever match.

@stianst stianst merged commit caebe50 into keycloak:main Apr 6, 2022
stianst pushed a commit to keycloak/keycloak-documentation that referenced this pull request Apr 6, 2022
@tyandor
Update changes-18_0_0.adoc (#1452)
f26ac2b 
Adding an entry for Account console Patternfly upgrade. PR - keycloak/keycloak#10748
@mabartos mabartos mentioned this pull request Apr 19, 2022
Merged
@cgeorgilakis cgeorgilakis mentioned this pull request May 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonkoops jonkoops jonkoops requested changes

@stianst stianst stianst approved these changes

@edewit edewit edewit left review comments

@ssilvert ssilvert ssilvert left review comments

Assignees

@ssilvert ssilvert

Labels
None yet
Projects
None yet
Milestone
18.0.0
Development

Successfully merging this pull request may close these issues.

Keycloak account console PatternFly design system alignment
5 participants
@tyandor @edewit @ssilvert @stianst @jonkoops
0