Comments 💬 (backend + frontend) #1153

tudi2d · 2021-05-17T10:41:06Z

PR Checklist

- Latest master branch merged
- PR title descriptive (can be used in release notes)

PR Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Description

First implementation of the comments to be merged.

Comments can't be edited from the FE site right now - they can only be deleted. This should be ok for the first release, but could be added afterwards. Admins & comment creators are allowed to remove comments.

Combine PR #1145 & #1146

Git Issues

Closes #1127 & Closes #1128

Screenshots/Videos

- Update to node-sass to work with node v14

- Change data flow to go down from HowTo component

…mments-combined

…ments-combined

- Optimize UX - Integrate "Show more"

tudi2d · 2021-05-17T10:42:37Z

@danitrod Your work made it really easy to build on! Thanks man! If you have any suggestions/things to be changed let me know :)

@davehakkens Here is the combined version of the PRs

github-actions · 2021-05-17T10:45:26Z

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

https://onearmy-next--pr1153-comments-combined-hca4yv9i.web.app

_{(expires Wed, 07 Jul 2021 05:11:49 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

davehakkens · 2021-05-17T14:12:25Z

yeahh nice work @tudi2d and @danitrod.

Few comments from my side after some testing
Important ones:
1. Deletion bug: In some way I ended up deleting your very controversial skateboard comment Philipp. I wanted to delete one of my own but yours seems to be removed as well. (was not logged in as admin)
2. Input field character limit Currently there is no limit. Should be set to 400 characters (original was 200 but might be to little)

Minor ones
3. Icon change The icon you see (currently the pink hand) should be in sync with your profile icon (workspace, collectionpoint, machineshop, member etc.) So it's the same as the one you see in the right top corner to go to your profile
4 Change location doesn't change flag If I change my location it changes the flag in my profilee But it doesn't change the flag on my comments. Ideally they are changed everywhere

danitrod · 2021-05-17T15:13:13Z

Thank you Dave, I'll tackle on points 1 and 4 as they're more related to back-end. Point 2 I think both of us can work on, I'll add a functionality for the back-end function to crop the comment to have length 400 if it comes with more than that, but the input field in the front-end should not allow more than 400 chars as well.

Is that ok @tudi2d?

danitrod · 2021-05-17T15:16:36Z

@davehakkens about 1, confirmed bug, I just accidentally deleted your test comments by deleting one of mine. I'll take a look into why that's happening.

…mments-combined

- Adjust current user avatar for commenting

tudi2d · 2021-05-18T17:29:15Z

Looks like this now.

davehakkens · 2021-05-18T22:26:03Z

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

https://onearmy-next--pr1153-comments-combined-hca4yv9i.web.app

(expires Thu, 17 Jun 2021 17:34:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Do you test it with that url? Doesn't seem to show any changes here..
Locally I can see them, looks good! But I can't post/test anything.

But perhaps it has to do with the Pull requests stacked on top of eachother?

tudi2d · 2021-05-19T09:40:04Z

I adjusted the styles - should look like intended now. Not sure about the error with posting things - @danitrod any idea?

davehakkens · 2021-05-19T12:23:48Z

nice looking good. One last thing for you @tudi2d Would be nice if you could align the text and remove the text limed when not logged in

danitrod · 2021-05-19T16:18:26Z

I adjusted the styles - should look like intended now. Not sure about the error with posting things - @danitrod any idea?

How did you get that error? I'm not being able to reproduce here

tudi2d · 2021-05-19T16:34:37Z

How did you get that error? I'm not being able to reproduce here

I used the version deployed on the preview URL & tried to enter a comment.

danitrod · 2021-05-19T16:55:56Z

how weird - for me this isn't happening; is there any more information in the error so I can better debug? Also if you could check what was the sent payload in the request from the network devtool that might help

- Center not logged in text

tudi2d · 2021-05-19T18:18:08Z

It looked like this:

I think, I fixed it for me - @davehakkens does it work for you now as well? The issue was, that the country was set to undefined & firebase does not like that (as undefined seems like a none intended, missing value - null is the way to go). Difficult to debug without the information indeed...

davehakkens · 2021-05-19T18:30:08Z

I'm testing with that url, not sure if that is the best way. But posting seems to work now.
However, the country flag still doesn't update here @danitrod?
Comments:

Profile:

davehakkens · 2021-05-19T18:43:57Z

Woops and I found one more thing @danitrod!
You are now showing the "Display name" and it should show the "Username".
so for instance Precious Plastic Headquarters should be precious-plastic
So it's consistent throughout the platform.

Sorry for not seeing this one sooner or making it more clear in the first place. 🙏

★ as a bonus you could also make it link to the user profile. But no need. We can also do it in an issue later

tudi2d · 2021-05-19T19:41:52Z

I will create an issue for the link to the profile, so it can be worked on after the PR is merged. It should be a good & easy follow up.

My suggestion would be to change the "Display name" & "Username" & than let @chrismclarke review the PR maybe, as this is already becoming a bigger PR. Not sure how hard fixing the flag issue is, but in my opinion a country change is (for the most users & workspaces) probably more of an edge case. It's great, that we already catched it & if it's not easy to fix it could also be a good follow up (as it is more of an improvement, than a bug to the comments). What do you guys think?

davehakkens · 2021-05-19T20:24:42Z

yeh good point, getting big indeed! Your proposal sounds good to me

danitrod · 2021-05-20T00:48:13Z

Nice catch @tudi2d ! Definitely difficult to debug, thank you. @davehakkens about updating information when the user updates, unfortunately the Firebase functions aren't deployed with the preview as far as I know, so we can't know if it's working yet - I left a note here #1146 (comment). We'll need Chris' help to check on how to test that.

I agree we this PR is getting a bit overloaded, I will update the comments to use username and not displayname and I guess we can merge after the review.

Edit: just pushed a commit to the back-end PR with the userName fix

- Center not logged in text

…mments-combined

tudi2d · 2021-05-20T08:57:20Z

Looks good! I liked, how we worked on this really asynchronously across time zones & still build it rather fast (for a free time project) - good communication & quality work from your site @danitrod! 🙂 Thanks for the feedback & testing @davehakkens!

chrismclarke · 2021-06-01T06:15:55Z

Sorry, I know I've been slow to review this, have got held up by a bunch of core build issues not working correctly. Will do my best to find time this week once the rest of the build is stable again.

chrismclarke

Really nice job here, thanks so much for all you've done @danitrod and @tudi2d.

The codebase looks pretty solid to me, I think there'll likely a few minor UI changes required and be good to get some more user input before going live so I'll add a beta-tester authwrapper to keep it only viewable by admins/beta-testers, but otherwise definitely happy to merge.

I really appreciate that you also took some time to think about challenges with things like users changing names - for me this is a major sticking point of the current database setup making these types of modules much harder to implement (than say, graphql where you could query and merge users names, countries etc. as you retrieve the comments). But I think what you've done is as good as we can hope for short term so thanks for finding a way to work in the constraints.

chrismclarke · 2021-06-07T04:21:59Z

functions/src/userUpdates/index.ts

 import { backupUser } from './backupUser'

 /*********************************************************************
 * Side-effects to be carried out on various user updates, namely:
 * - create user revision history
 * - update map pin location on change
 * - update howTo creator flag on change
+ * - update userName and flag on any comments made by user


I didn't know you could even change username! I think this leaves us with a few challenges to figure out, but good to have included in the comment update. Longer term we will probably need a refactor to keep user id independent of user name (I can't remember why we didn't do that in the first place - possibly to make serving the avatar image easier)... but in any case good to have this here for now

chrismclarke · 2021-06-07T04:26:54Z

src/models/howto.models.tsx

@@ -11,7 +23,10 @@ export interface IHowto extends IHowtoFormInput, IModerable {
  cover_image: IUploadedFileMeta
  files: Array<IUploadedFileMeta | File | null>
  steps: IHowtoStep[]
+  // Comments were added in V2, old howto's may not have the property
+  comments?: IComment[]


Thanks for adding the comment to explain this

chrismclarke · 2021-06-07T04:33:28Z

src/components/Comment/CommentTextArea.tsx

+  const user = stores.userStore.activeUser
+
+  async function keyDown(e) {
+    if (e.key === 'Enter' && !e.shiftKey && comment.trim().length) {


Nice idea to add this, would definitely expect pressing the enter key to submit. I'm not sure if you can achieve the same functionality using simple form elements (e.g. https://stackoverflow.com/questions/33211672/how-to-submit-a-form-using-enter-key-in-react-js/33212911), but happy to keep this in to be more explicit (and add the empty check)

chrismclarke · 2021-06-07T04:36:46Z

src/stores/Howto/howto.store.tsx

@@ -136,6 +138,107 @@ export class HowtoStore extends ModuleStore {
    return needsModeration(howto, toJS(this.activeUser))
  }



I think likely we will want to reuse comments in other pages, so might be better to try and separate out into its own store or set of methods. But happy to keep here for now and can always refactor later.

…into pr/tudi2d/1153

tudi2d and others added 10 commits May 6, 2021 22:39

Init basic comments components

2bc82bb

- Update to node-sass to work with node v14

Add basic styling & margins

c343f69

add comment backend for howtos

ade7a6e

comments: fix country storing

92b9965

Finalize design

098a1f7

- Change data flow to go down from HowTo component

Merge remote-tracking branch 'danitrod/1128-comments-backend' into co…

eef36a0

…mments-combined

Remove debug components

dbea8ad

Merge remote-tracking branch 'origin/1127-comments-frontend' into com…

b694e1f

…ments-combined

Adjust props & typings

1d30e92

Add comment add & delete function

01ef7df

- Optimize UX - Integrate "Show more"

tudi2d added Review: Assigned 👉 Waiting on review from a specific dev Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview labels May 17, 2021

tudi2d requested a review from chrismclarke May 17, 2021 10:41

tudi2d self-assigned this May 17, 2021

github-actions bot added the Mod: HowTo 📰 label May 17, 2021

fix bugs pointed out by dave in ONEARMY#1153

31cc21a

danitrod mentioned this pull request May 18, 2021

Comments backend #1146

Merged

5 tasks

tudi2d added 2 commits May 18, 2021 12:19

Merge remote-tracking branch 'danitrod/1128-comments-backend' into co…

40a8561

…mments-combined

Limit Comment input to 400 characters

5e36e07

- Adjust current user avatar for commenting

github-actions bot added the Backend label May 18, 2021

tudi2d added Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview and removed Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview Backend labels May 18, 2021

Adjust TextArea styling

719c4af

github-actions bot added the Backend label May 19, 2021

Prevent country from beein undefined

282c3c1

- Center not logged in text

tudi2d mentioned this pull request May 19, 2021

[feature request] Link username in comment to profile #1157

Closed

tudi2d and others added 3 commits May 19, 2021 22:01

Prevent country from beein undefined

ba6d188

- Center not logged in text

comments: change displayName to userName

85fa8f7

Merge remote-tracking branch 'danitrod/1128-comments-backend' into co…

00a207c

…mments-combined

chrismclarke approved these changes Jun 7, 2021

View reviewed changes

chrismclarke added 3 commits June 6, 2021 21:55

add auth wrapper for beta-testers

80d5359

add cypress tag

7d1746e

Merge branch 'master' of https://github.com/ONEARMY/community-platform …

4fb978b

…into pr/tudi2d/1153

chrismclarke merged commit 87e5f0b into ONEARMY:master Jun 7, 2021

davehakkens mentioned this pull request Dec 29, 2021

[bug] Flags do not appear on Where are you based? input #1351

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments 💬 (backend + frontend) #1153

Comments 💬 (backend + frontend) #1153

tudi2d commented May 17, 2021 •

edited

Loading

tudi2d commented May 17, 2021

github-actions bot commented May 17, 2021 •

edited

Loading

davehakkens commented May 17, 2021

danitrod commented May 17, 2021

danitrod commented May 17, 2021

tudi2d commented May 18, 2021

davehakkens commented May 18, 2021

tudi2d commented May 19, 2021

davehakkens commented May 19, 2021

danitrod commented May 19, 2021

tudi2d commented May 19, 2021

danitrod commented May 19, 2021

tudi2d commented May 19, 2021

davehakkens commented May 19, 2021

davehakkens commented May 19, 2021

tudi2d commented May 19, 2021 •

edited

Loading

davehakkens commented May 19, 2021

danitrod commented May 20, 2021 •

edited

Loading

tudi2d commented May 20, 2021

chrismclarke commented Jun 1, 2021

chrismclarke left a comment

chrismclarke Jun 7, 2021

chrismclarke Jun 7, 2021

chrismclarke Jun 7, 2021

chrismclarke Jun 7, 2021

		@@ -136,6 +138,107 @@ export class HowtoStore extends ModuleStore {
		return needsModeration(howto, toJS(this.activeUser))
		}

Comments 💬 (backend + frontend) #1153

Comments 💬 (backend + frontend) #1153

Conversation

tudi2d commented May 17, 2021 • edited Loading

Description

Git Issues

Screenshots/Videos

tudi2d commented May 17, 2021

github-actions bot commented May 17, 2021 • edited Loading

davehakkens commented May 17, 2021

danitrod commented May 17, 2021

danitrod commented May 17, 2021

tudi2d commented May 18, 2021

davehakkens commented May 18, 2021

tudi2d commented May 19, 2021

davehakkens commented May 19, 2021

danitrod commented May 19, 2021

tudi2d commented May 19, 2021

danitrod commented May 19, 2021

tudi2d commented May 19, 2021

davehakkens commented May 19, 2021

davehakkens commented May 19, 2021

tudi2d commented May 19, 2021 • edited Loading

davehakkens commented May 19, 2021

danitrod commented May 20, 2021 • edited Loading

tudi2d commented May 20, 2021

chrismclarke commented Jun 1, 2021

chrismclarke left a comment

Choose a reason for hiding this comment

chrismclarke Jun 7, 2021

Choose a reason for hiding this comment

chrismclarke Jun 7, 2021

Choose a reason for hiding this comment

chrismclarke Jun 7, 2021

Choose a reason for hiding this comment

chrismclarke Jun 7, 2021

Choose a reason for hiding this comment

tudi2d commented May 17, 2021 •

edited

Loading

github-actions bot commented May 17, 2021 •

edited

Loading

tudi2d commented May 19, 2021 •

edited

Loading

danitrod commented May 20, 2021 •

edited

Loading