Account Profile rewritten #18076

gabriellsh · 2020-06-30T00:15:54Z

Proposed changes

Rewrite User Profile Page using React

Issue(s)

How to test or reproduce

Screenshots

Types of changes

Bugfix (non-breaking change which fixes an issue)
Improvement (non-breaking change which improves a current function)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Hotfix (a major bugfix that has to be merged asap)
Documentation Update (if none of the other choices apply)

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works (if applicable)
I have added necessary documentation (if applicable)
Any dependent changes have been merged and published in downstream modules

Changelog

Further comments

lgtm-com · 2020-06-30T00:26:07Z

This pull request introduces 4 alerts and fixes 2 when merging 8cde558 into 0c26dca - view on LGTM.com

new alerts:

2 for Duplicate HTML element attributes
2 for Useless assignment to property

fixed alerts:

1 for Duplicate HTML element attributes
1 for Useless assignment to property

lgtm-com · 2020-06-30T17:52:54Z

This pull request introduces 4 alerts and fixes 2 when merging 8cde558 into 1157075 - view on LGTM.com

new alerts:

2 for Duplicate HTML element attributes
2 for Useless assignment to property

fixed alerts:

1 for Duplicate HTML element attributes
1 for Useless assignment to property

…rewrite_account_profile

…into new/rewrite_account_profile * 'rewrite/accounts' of github.com:RocketChat/Rocket.Chat: [FIX] Avatar ETag missing from User (#18109) Handle callback returning promise. (#18102) [FIX] Not possible to read encrypted messages after disable E2E on channel level (#18101)

tassoevan · 2020-07-03T21:36:28Z

client/components/basic/avatar/UserAvatar.js

+}, [url, username, userId]);
+
+function UserAvatar({ url, username, userId, ...props }) {
+	const avatarUrl = useAvatarUrl({ url, username, userId });


Why not lift this hook up to the parent? Instead of having two mutually exclusive props (url and userId) that can be accidentally set together, it can receive only url.

In my opinion, the idea of having a specific component like UserAvatar should allow me to not bother about how I get the URL. If we make it that only the URL is accepted, the behaviour won't differ from the Avatar component from fuselage.

I agree it's great to have some encapsulation, but it should be closely-knit. If I've the option to pass the user ID, then why did I need to set username too if it can be resolved the same way url is? If the answer is "sometimes you already have it and we don't need to recompute", then this url and username resolution shouldn't really be here.

As an example, in UserAvatar is a dumb component in UserAvatarEditor when it serves as a view for suggestions, but also it's used as the de facto component to show user avatar. If UserAvatar must have some logic to show the avatar for a user, then Avatar should be used elsewhere.

…into new/rewrite_account_profile * 'rewrite/accounts' of github.com:RocketChat/Rocket.Chat: [FIX] Cannot open admin when server uses ROOT_URL with subpath (#18105) (#18147) [FIX] App details returns to apps table, instead of previous page (#18080) [FIX] Clipboard not working when permalinking a pinned message (#18047) [FIX] "Add reaction" icon missing when the viewport size is smaller than 500px (#18110) [FIX] Jitsi opening twice (#18111) [FIX] Email notifications were still being sent for online users (#18088) [FIX] The livechat agent activity monitor wasn't being initialised because due to an internal error (#18090)

client/account/AccountProfileForm.js

client/account/AccountProfilePage.js

app/threads/client/components/ThreadListMessage.js

client/account/AccountProfileForm.js

client/account/AccountProfilePage.js

client/components/basic/userStatus/UserStatus.js

client/account/AccountRoute.js

client/admin/users/EditUser.js

client/admin/users/UserForm.js

tassoevan · 2020-07-13T22:38:38Z

client/components/basic/avatar/UserAvatarEditor.js


-export function UserAvatarEditor({ username, setAvatarObj }) {
+function UserAvatarSuggestions({ suggestions, setAvatarObj, setNewAvatarSource, disabled, ...props }) {


Prefer event listeners (on*) over setters (set*) passed through props.

tassoevan · 2020-07-13T22:43:37Z

client/components/basic/avatar/UserAvatarEditor.js

+		setAvatarObj(suggestion);
+		setNewAvatarSource(suggestion.blob);


If these setters are called together, it's interesting to trigger a single event for them, the reason being each setter call its own component update and we should let the parent component to decide if it two updates are needed or not (e.g. a single dispatch call from a useReducer hook).

client/helpers/getUserEmailAddress.js

Co-authored-by: Tasso Evangelista <[email protected]>

…ocket.Chat into new/rewrite_account_profile

lgtm-com · 2020-07-14T03:41:41Z

This pull request introduces 4 alerts when merging 8aa5c56 into b7668c5 - view on LGTM.com

new alerts:

4 for Unused variable, import, function or class

ggazzo changed the base branch from develop to rewrite/accounts June 30, 2020 17:36

gabriellsh added 4 commits June 30, 2020 15:16

Rewrite user Profile

cec3cd3

Removed old Files

bfc24a8

Remove unused module

4dac36f

Fix rebase

796d08b

gabriellsh force-pushed the new/rewrite_account_profile branch from 8cde558 to 796d08b Compare June 30, 2020 18:24

gabriellsh and others added 6 commits June 30, 2020 15:36

Change buttons

f30e590

Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into new/…

e3cd384

…rewrite_account_profile

Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into new/…

16f4f47

…rewrite_account_profile

Review

5f39780

Use avatar ETag.

4eb49cc

tassoevan requested changes Jul 3, 2020

View reviewed changes

tassoevan changed the title ~~chore: Rewrite account profile~~ [NEW] Account Profile rewritten Jul 7, 2020

tassoevan requested changes Jul 7, 2020

View reviewed changes

client/account/AccountProfileForm.js Show resolved Hide resolved

client/account/AccountProfilePage.js Outdated Show resolved Hide resolved

tassoevan requested changes Jul 7, 2020

View reviewed changes

client/components/basic/userStatus/UserStatus.js Outdated Show resolved Hide resolved

Review WIP

5402457

tassoevan added ui/ux type: improvement labels Jul 13, 2020

tassoevan added this to the 3.5.0 milestone Jul 13, 2020

gabriellsh force-pushed the new/rewrite_account_profile branch from 5402457 to 630dafc Compare July 13, 2020 15:49

tassoevan self-requested a review July 13, 2020 16:40

Review

3a5689e

gabriellsh force-pushed the new/rewrite_account_profile branch from 630dafc to 3a5689e Compare July 13, 2020 17:45

tassoevan requested changes Jul 13, 2020

View reviewed changes

client/account/AccountRoute.js Outdated Show resolved Hide resolved

client/admin/users/EditUser.js Outdated Show resolved Hide resolved

tassoevan reviewed Jul 13, 2020

View reviewed changes

client/admin/users/UserForm.js Outdated Show resolved Hide resolved

tassoevan requested changes Jul 13, 2020

View reviewed changes

client/helpers/getUserEmailAddress.js Outdated Show resolved Hide resolved

tassoevan mentioned this pull request Jul 14, 2020

[NEW] Personal Access Tokens rewritten #18206

Merged

12 tasks

ggazzo and others added 3 commits July 13, 2020 23:57

Update client/helpers/getUserEmailAddress.js

76fcd1c

Co-authored-by: Tasso Evangelista <[email protected]>

Fix review

2a99ab4

Merge branch 'new/rewrite_account_profile' of github.com:RocketChat/R…

8aa5c56

…ocket.Chat into new/rewrite_account_profile

tassoevan added 4 commits July 14, 2020 01:05

Fix UserAvatar usage

14144e9

Remove unused imports

05c76d2

Relax spacing in UserStatusMenu

083c314

Harden layout positioning in AccountProfileForm

234d56a

tassoevan approved these changes Jul 14, 2020

View reviewed changes

ggazzo changed the title ~~[NEW] Account Profile rewritten~~ Account Profile rewritten Jul 14, 2020

ggazzo merged commit 96b094a into rewrite/accounts Jul 14, 2020

ggazzo deleted the new/rewrite_account_profile branch July 14, 2020 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account Profile rewritten #18076

Account Profile rewritten #18076

gabriellsh commented Jun 30, 2020 •

edited by ggazzo

Loading

lgtm-com bot commented Jun 30, 2020

lgtm-com bot commented Jun 30, 2020

tassoevan Jul 3, 2020

gabriellsh Jul 7, 2020

tassoevan Jul 7, 2020

tassoevan Jul 7, 2020

tassoevan Jul 13, 2020

tassoevan Jul 13, 2020 •

edited

Loading

lgtm-com bot commented Jul 14, 2020


		export function UserAvatarEditor({ username, setAvatarObj }) {
		function UserAvatarSuggestions({ suggestions, setAvatarObj, setNewAvatarSource, disabled, ...props }) {

		setAvatarObj(suggestion);
		setNewAvatarSource(suggestion.blob);

Account Profile rewritten #18076

Account Profile rewritten #18076

Conversation

gabriellsh commented Jun 30, 2020 • edited by ggazzo Loading

Proposed changes

Issue(s)

How to test or reproduce

Screenshots

Types of changes

Checklist

Changelog

Further comments

lgtm-com bot commented Jun 30, 2020

lgtm-com bot commented Jun 30, 2020

tassoevan Jul 3, 2020

Choose a reason for hiding this comment

gabriellsh Jul 7, 2020

Choose a reason for hiding this comment

tassoevan Jul 7, 2020

Choose a reason for hiding this comment

tassoevan Jul 7, 2020

Choose a reason for hiding this comment

tassoevan Jul 13, 2020

Choose a reason for hiding this comment

tassoevan Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jul 14, 2020

gabriellsh commented Jun 30, 2020 •

edited by ggazzo

Loading

tassoevan Jul 13, 2020 •

edited

Loading