Page MenuHomePhabricator

[web] Introduce placeholder `WebEditUserAvatarProvider`
ClosedPublic

Authored by atul on Jun 19 2023, 2:55 PM.
Tags
None
Referenced Files
F3413583: D8249.id27899.diff
Thu, Dec 5, 5:32 AM
F3413561: D8249.id27898.diff
Thu, Dec 5, 5:26 AM
F3413447: D8249.id27902.diff
Thu, Dec 5, 5:01 AM
F3413434: D8249.id27865.diff
Thu, Dec 5, 4:59 AM
F3413394: D8249.id27899.diff
Thu, Dec 5, 4:48 AM
F3413338: D8249.id27898.diff
Thu, Dec 5, 4:42 AM
F3413084: D8249.id27902.diff
Thu, Dec 5, 4:18 AM
F3413069: D8249.id27865.diff
Thu, Dec 5, 4:14 AM
Subscribers

Details

Summary

Very very naive implementation of WebEditUserAvatarProvider w/ platform-specific props set to dummy values for the timebeing.

This unblocks (non-image) avatar work (remove avatar, emoji avatar, etc) and lets us defer the image avatar stuff for later. Will obviously have to come back and implement these when it comes time to work on image avatar work, just want to start w/ the simplest flows first.

Test Plan

Made sure I could "get values out of the context" from EditUserAvatar w/ some logging:

2bb979.png (390×1 px, 116 KB)

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

web/app.react.js
217–253 ↗(On Diff #27866)

Not sure that this is the right place to put WebEditUserAvatarProvider in the React component tree.

Here's my understanding:

  • On native, we have most of our providers in Root() at "the top level" of the application.
  • On web, we have (fewer) providers wrapping {content} in App():

c8fc72.png (1×1 px, 234 KB)

as well as some that are more "local" to the components from where they're consumed, eg:

d5f268.png (294×1 px, 62 KB)

  • It's generally considered good for performance to "push providers down" to limit scope and reduce re-renders.
  • However, in this situation I think it makes sense to have WebEditUserAvatarProvider "as high as possible" because we may consume it in other parts of the application (eg if the user avatar is shown anywhere else on the page in the future and we want a spinner to indicate that it's changing or whatever).
  • Since content in app.react.js depends on loggedIn, I think it makes sense to have the provider "within" this.renderMainContent()... since that's where a UserAvatar is relevant anyways:

7503ec.png (346×830 px, 47 KB)

  • Within this.renderMainContent(), it makes sense to wrap header (maybe we show user avatar in header one day?), mainContent, and maybe CommunityPicker within the provider.

So I thought it made sense to wrap what's returned by this.renderMainContent() with the provider. However, that doesn't feel right. I would expect something more like native where the provider is included within a deeply nested "triangle" of providers.

Any suggestions on a more appropriate place to include this provider or some other way I should be thinking about this?

atul published this revision for review.Jun 19 2023, 3:21 PM
atul attached a referenced file: F594252: 7503ec.png. (Show Details)
atul attached a referenced file: F594250: d5f268.png. (Show Details)
atul attached a referenced file: F594248: c8fc72.png. (Show Details)

Seems reasonable

web/avatars/web-edit-user-avatar-provider.react.js
8 ↗(On Diff #27866)

Can we use a new Unicode right-hand quote character here?

This revision is now accepted and ready to land.Jun 19 2023, 5:08 PM
web/avatars/web-edit-user-avatar-provider.react.js
8 ↗(On Diff #27866)

Yeah, will update

rebase before addressing feedback

rebase after addressing feedback

This revision was landed with ongoing or failed builds.Jun 20 2023, 9:53 AM
This revision was automatically updated to reflect the committed changes.