feat(discussions): add avatars

[benfurber]

PR Checklist

• - Commit messages are descriptive, it will be used in our Release Notes

PR Type

• New feature (non-breaking change which adds functionality)

Description

Add option user profile image to comments (and tidies up various responsive layout stuff).

Screenshots/Videos

[davehakkens]

Niceeee! Could we display this functionality only to beta-testers first? Since it might bring some weird UI changes

[github-actions]

Visit the preview URL for this PR (updated for commit c97ad47):

https://onearmy-next--pr3587-feat-discussion-add-7gevp0ga.web.app

_{(expires Wed, 03 Jul 2024 14:42:01 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

[benfurber]

@davehakkens, et al. I'm not finished on this branch so doesn't need a review yet.

[davehakkens]

well i'm looking anyway 🤓

[benfurber]

Niceeee! Could we display this functionality only to beta-testers first? Since it might bring some weird UI changes

Sorry @davehakkens, I missed this comment yesterday. I think based on the current beta tester wrapper component and approach, a fair amount of extra re-coding would be needed to enable them to evaulate this change on prod.

Maybe instead the preview link/branch could be left open for longer to get tested by a broader group before merging?

[davehakkens]

It's not a matter of adding an auth wrapper to only display the avatar to beta-testers? (It should be easy to add this)
I would prefer to test features like this as beta-tester:

• Avoid things being released that are buggy (feels like we had a little to much of that recently)
• This is the first time visually avatars are shown. But on the profile settings this isnt very well intergrated yet (workspaces use landscape images and others can be weirdly cropped) could forsee a bunch scenarios coming that could be fixed upfront in beta-test.
• Test with live data, the dev-site isnt a good representation
• A broader group of users will see/test it then devs. Which gives more eyes and feedback from other devices and browsers.

[benfurber]

@davehakkens I take all your points.

It's more work, because:

1. We add the beta tester wrapper at the app code level while the avatar addition code is nested five(ish) components deep in the component library.
2. The beta tester wrapper layout stylings is a little clunky for small elements.
3. The wrapper currently adds a 4px border on each side. So that's 8 per axis. As currently coded the avatar is only 30x30 for the mobile display. So if I put the wrapper litterally right around the image, it would changes the experience of feature.

So I'll:

1. Add a property from the DiscussionContainer on down to the CommentItem to set whether to show the avatar or not.
2. Add the beta tester wrapper at the DiscussionContainer level so that the wrapper border doesn't get in the way of experiencing the feature.

[davehakkens]

Ah i see. Thanks for explaining @benfurber
Your plan sounds good to me!

[codecov]

Codecov Report

Attention: Patch coverage is 87.43719% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 68.40%. Comparing base (2f9cc11) to head (c97ad47).
Report is 27 commits behind head on master.

Files	Patch %	Lines
...ackages/components/src/CommentItem/CommentItem.tsx	87.06%	15 Missing ⚠️
...components/src/CommentItem/CommentItem.stories.tsx	69.23%	4 Missing ⚠️
...iscussionContainer/DiscussionContainer.stories.tsx	40.00%	3 Missing ⚠️
src/stores/Discussions/discussions.store.tsx	77.77%	2 Missing ⚠️
...ctions/src/userUpdates/updateDiscussionComments.ts	95.23%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3587    +/-   ##
========================================
  Coverage   68.40%   68.40%            
========================================
  Files         445      449     +4     
  Lines       13849    14010   +161     
  Branches     2479     2499    +20     
========================================
+ Hits         9473     9584   +111     
- Misses       4333     4383    +50     
  Partials       43       43            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cypress]

2 flaky tests on run #5725 ↗︎

0

77

1

0

2

Details:

refactor: rename function for convention
Project: onearmy-community-platform	Commit: c97ad4738c
Status: Passed	Duration: 04:56 💡
Started: Jun 3, 2024 12:35 PM	Ended: Jun 3, 2024 12:40 PM

  research/write.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
[Research] > [Create research article] > [By Authenticated]		Test Replay Screenshots Video

  howto/read.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
... > [See all info]		Test Replay Screenshots Video

Review all test suite changes for PR #3587 ↗︎

[benfurber]

On Mobile

Before

Now

For reg users

BT: When no images present

BT: With images

[pizzaisdavid]

General remark about what I am seeing in the codebase and this pull request: it seems that it is a common case that something changes about a user, for example image (or location) and then their contributions need to be updated (via handleUserUpdates, I think.)

My first thought is that it would be less of a headache to only store the id of a user and the content of their comment. And then, when we want to show the avatar (or location) the id is used to lookup the user and then get their current avatar, merging the two together (either on the backend when requesting discussion data or on the frontend.)

All this logic about if the image changed could go away. So it would just be that the user can change their avatar as much as they want and only the data in the user collection would change, discussions would not get updated at all.

Maybe there is some Firebase limitation/justification or some other good reason. 😄

[mariojsnunes]

Maybe there is some Firebase limitation/justification or some other good reason. 😄

Yes its a common practice for a NoSql database.
If we eventually migrate to supabase which uses postgres, we would implement the approach you described

[pizzaisdavid]

I worked at a company that used Mongodb (which is NoSQL) and they were doing the method I suggested. But on some further research, it is kinda like they were treating NoSQL like SQL (strict validation on fields via Mongoose, using stuff like foreign keys, no duplicate data spread across the database.)

But I have found this video series, which uses a very similar example to the usecase here (user submissions and wanting to show profile pictures.) https://youtu.be/v_hR4K4auoQ?list=PLl-K7zZEsYLluG5MCVEzXAQ7ACZBCuZgZ&t=380 (timstamped for convenience) I guess it is a general trend of NoSQL and the approach optimizes reads over edits, which makes sense. As the edits don't happen often. And something something about Firebase billing.

Consider my comment resolved. Thanks for the push to learn more @mariojsnunes 😄

[benfurber]

Thanks @mariojsnunes and @pizzaisdavid. Any more code level comments?

[onearmy-bot]

🎉 This PR is included in version 1.188.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀