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

feat: new profile page and PATs front-end #2109

Merged
merged 19 commits into from
Oct 3, 2022
Merged

feat: new profile page and PATs front-end #2109

merged 19 commits into from
Oct 3, 2022

Conversation

nunogois
Copy link
Member

@nunogois nunogois commented Sep 28, 2022

https://linear.app/unleash/issue/2-278/user-profile-page-front-end

Due to the way the work flowed, also includes: https://linear.app/unleash/issue/2-274/frontend-for-personal-api-tokens - I can split it into a separate PR if it's easier to review.

Also includes:

  • TimeAgoCell title refactoring;

@vercel
Copy link

vercel bot commented Sep 28, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
unleash-docs ✅ Ready (Inspect) Visit Preview Oct 3, 2022 at 9:35AM (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview Oct 3, 2022 at 9:35AM (UTC)

@nunogois nunogois marked this pull request as ready for review September 29, 2022 15:59
@nunogois nunogois changed the title feat: new user dropdown and profile page feat: new profile page and PATs front-end Sep 29, 2022
Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

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

Looks super clean! Minor comments and suggestions.

From a UI perspective, should the password page use PageContent to get the same appearance as the PAT page? Could also consider this for the Main profile page content. @NicolaeUnleash ?

@nicolaesocaciu
Copy link
Contributor

@FredrikOseberg yes the password page should use PageContent to get the same appearance
On the other hand, profile page could be a bit more customized. I will give it a try with Nuno to see how it will look with PageContent, but if it's not working properly we will at least adjust the paddings to align better with the rest

@nunogois
Copy link
Member Author

Included fixes, refactoring and any other misc changes related to https://linear.app/unleash/issue/2-287/ui-adjustments-for-personal-api-token as well.

Copy link
Contributor

@sjaanus sjaanus left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@FredrikOseberg FredrikOseberg left a comment

Choose a reason for hiding this comment

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

LG, I had one suggestion you could choose to follow or not.

Comment on lines 59 to 66
try {
const path = formatApiPath('api/admin/user/change-password');
const res = await fetch(path, {
await fetch(path, {
headers,
body: JSON.stringify({ password, confirmPassword }),
method: 'POST',
credentials: 'include',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be moved into an API hook

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed in 4160084

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

Should we use flagResolver to resolve this, the same way we do for embedded proxy?

Copy link
Contributor

@chriswk chriswk left a comment

Choose a reason for hiding this comment

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

I know Fredrik pointed out that there was an interaction that should probably be extracted into a hook. But the logic otherwise seems sound. 👍

@nunogois nunogois merged commit ddcfe13 into main Oct 3, 2022
@nunogois nunogois deleted the user-profile branch October 3, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants