feat: supports contributors on research item #2006 by thisislawatts · Pull Request #2007 · ONEARMY/community-platform

thisislawatts · 2022-11-25T14:15:44Z

Still to be done:

Testing
Updated type definitions
Enrich usernames to complete User objects before rendering
Design detailing on appearance in ResearchDescription

PR Checklist

- Commit messages are descriptive, it will be used in our 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)
Developer experience (improves developer workflows for contributing to the project)

Git Issues

Closes #2006, #2143

Note on implementation

Introduces an array item to Research articles which contains a list
of usernames. This is a deliberately naive implementation as Research
articles are currently only available on the Project Kamp instance.

Future development here may involve aligning with the UserReference
pattern used to persist @mention references. The UserReference
convention is more robust as it uses IDs rather than usernames to
identify users. The additional complexity is difficult to justify
in this instance as the target user story here only covers a few
Project Kamp contributors.

cypress · 2022-11-25T14:31:51Z

4 flaky tests on run #3063 ↗︎

0	78	3	0	4

Details:

Merge branch 'master' into feat/support-contributors-on-research-item
Project: onearmy-community-platform	Commit: `3a590769e8`
Status: Passed	Duration: 04:08 💡
Started: Mar 22, 2023 10:38 AM	Ended: Mar 22, 2023 10:42 AM

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

View Output Video

Test		Artifacts
[How To] > [Edit a how-to] > [By Authenticated]		`Output` `Screenshots`

notifications.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
[Notifications] > [are generated by adding comments to research]		`Output` `Screenshots`

settings.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
[Settings] > [Focus Machine Builder] > [Edit a new profile]		`Output` `Screenshots`

common.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
[Common] > [Page Navigation]		`Output` `Screenshots`

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

github-actions · 2022-12-22T00:11:35Z

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

https://onearmy-next--pr2007-feat-support-contrib-pc452dqi.web.app

_{(expires Fri, 21 Apr 2023 21:16:07 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

thisislawatts · 2022-12-30T10:56:16Z

@davehakkens what do you think. Would it be worth continuing work on this item?

davehakkens · 2022-12-30T12:39:42Z

For sure! I think this is a very useful PR. Specially later if it would also have notifications 🔥

davehakkens · 2023-02-10T20:24:27Z

what do you think about wrapping up this one @thisislawatts?
Seems possible now this one is merged? #2056

thisislawatts · 2023-02-11T13:21:09Z

How do we think Research contributors should interact with the Notifications behaviour?

Would a Research contributor want any of the following messages:

A notification when a new comment has been added to the Research article
A notification when a research article has been marked as Useful

davehakkens · 2023-02-11T13:25:22Z

* A notification when a new comment has been added to the Research article

* A notification when a research article has been marked as Useful

Both of those are very interesting for contributors to receive.
To follow up conversation and see what they did was useful
Basically same as the original author.

You trying to implement notifications in this PR as well? 🔥

thisislawatts · 2023-02-11T21:37:43Z

I think let's raise a separate PR, integrating this with the notifications mechanism. This PR has been standing open for long enough.

Edit: It looks like the Research notifications PR has not been merged yet.

thisislawatts · 2023-02-12T14:41:10Z

Could I get some direction from @ONEARMY/design on presentation for a list of contributors, the attached screengrab shows how this would look with one named contributor davetestuser.

davehakkens · 2023-02-12T14:48:32Z

Would it be possible to start by adding it only in the new CTA for now?
There is more space available and it would fit well. Like in the design here #2053:

thisislawatts · 2023-02-12T15:01:19Z

🤦🏻 Of course, thanks @davehakkens! I totally forgot about that.

davehakkens · 2023-02-15T16:53:09Z

Oee is this one ready for review @thisislawatts? :)

davehakkens · 2023-02-15T19:08:31Z

Nice! Works good here
Any specific reason to change the text from With contributions from to In collaboration with.
Contributions seems more suited since its often individuals contributing a part of the whole. But I could be wrong?

Could we either way change the text size to 14px instead of 12px to make it match with the username

davehakkens · 2023-02-15T19:10:45Z

Also any suggestions or references to link to when writing a follow up issue? >
That contributors receive a noticiation like mentioned above

thisislawatts · 2023-02-16T06:28:52Z

Thanks for taking a look @davehakkens, I've updated the wording and increased the fontSize.

davehakkens · 2023-02-16T22:39:10Z

You up for reviewing this one @asheerrizvi ?

asheerrizvi · 2023-02-21T11:17:51Z

@davehakkens sure

Introduces an array item to Research articles which contains a list of usernames. This is a deliberately naive implementation as Research articles are currently only available on the Project Kamp instance. Future development here may involve aligning with the UserReference pattern used to persist @mention references. The UserReference convention is more robust as it uses IDs rather than usernames to identify users. The additional complexity is difficult to justify in this instance as the target user story here only covers a few Project Kamp contributors.

chrismclarke

Many thanks @thisislawatts, all the functionality looks good to me and I really like the UI also.

I've added a couple general thoughts in-line, but nothing that blocks this PR so I'd say good to go.

Thanks for the contribution!

chrismclarke · 2023-03-22T12:51:00Z

src/pages/Research/Content/Common/Research.form.tsx

+                              </Label>
+                              <Field
+                                name="collaborators"
+                                component={FieldInput}


nit(non-blocking)
I could imagine a nice feature in the future to have some sort of autofill/lookup as you start typing names, but given that would need some sort of aggregation for all usernames or other server-side mechanism definitely beyond scope of this PR.

chrismclarke · 2023-03-22T13:07:46Z

src/pages/Research/Content/ResearchArticle.tsx


+    const collaborators = Array.isArray(item.collaborators)
+      ? item.collaborators
+      : ((item.collaborators as string) || '').split(',').filter(Boolean)


nit(non-blocking)
Do we need the catch for string-formatted collaborators? I think it's just this PR adding, so hopefully that means in production nothing should be malformed (unless it relates to the input field which I know is a string?). Happy to keep for now in either case

chrismclarke · 2023-03-22T13:16:33Z

src/pages/Research/Content/ResearchListItem.tsx

+                      {collaborators.length +
+                        (collaborators.length === 1
+                          ? ' contributor'
+                          : ' contributors')}


nit(non-blocking)
Not really related to this PR, but at some point it might be worth thinking how we can make a bit more consistent use of pluralization. Perhaps a utility function that takes inputs for base word and singular/plural modifiers? e.g. textPlural(collaborators.length,'contributor','contributors')?

I also quite like the syntax in https://www.npmjs.com/package/simplur as an option, and am aware of more heavyweight options like pluralize (but don't think we really need something as comprehensive)

Let me know if you think it's worth making a follow-up issue on this (I can flag on maintainer call and add to git), or if you think makes more sense just to stick to lightweight inline functions

Good idea, I think it would be useful to introduce a helper method for this. I think either simplur or something like it would be very useful. Thanks for the link I had not seen these tagged functions before, very nice 💅🏻

Perfect, I'll add it to next month's discussion and open an issue for it following.

Yeah a definitely blindspot for me too, been using things like graphql{....} , html<div>...</div> for years but never stopped to wonder why it worked (just naively assumed it was something to do with vscode extensions and intellisense, makes a lot more sense that it's a supported syntax)

chrismclarke · 2023-03-22T13:18:28Z

src/stores/Research/research.store.tsx

+          newItem._createdBy,
+          '/research/' + newItem.slug + '#update_' + existingUpdateIndex,
+        )
+


Thanks for tidying this up, agree makes more sense to have the logic for this in the store

davehakkens · 2023-03-22T14:54:52Z

awesome. good to see this merged! Thanks @chrismclarke and @thisislawatts

onearmy-bot · 2023-03-22T21:10:48Z

🎉 This PR is included in version 1.41.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot added the Research 🧪 label Nov 25, 2022

thisislawatts self-assigned this Nov 25, 2022

thisislawatts force-pushed the feat/support-contributors-on-research-item branch 5 times, most recently from 664b175 to 1c78806 Compare November 25, 2022 21:34

davehakkens mentioned this pull request Dec 6, 2022

[feature request] Support contributors to Research items #2006

Closed

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Dec 22, 2022

davehakkens mentioned this pull request Dec 30, 2022

Add CTA for How-to and Research #2053

Closed

thisislawatts force-pushed the feat/support-contributors-on-research-item branch 3 times, most recently from f326231 to 9e128ef Compare February 12, 2023 13:54

thisislawatts marked this pull request as ready for review February 13, 2023 11:32

thisislawatts requested a review from a team as a code owner February 13, 2023 11:32

thisislawatts force-pushed the feat/support-contributors-on-research-item branch 3 times, most recently from 2a0e340 to 1b0d792 Compare February 13, 2023 11:39

thisislawatts force-pushed the feat/support-contributors-on-research-item branch from 1b0d792 to 9805425 Compare February 16, 2023 06:29

thisislawatts force-pushed the feat/support-contributors-on-research-item branch 5 times, most recently from b5d89df to 0fb954e Compare February 19, 2023 18:30

davehakkens requested a review from asheerrizvi February 20, 2023 21:59

thisislawatts force-pushed the feat/support-contributors-on-research-item branch from 0fb954e to 79338e1 Compare February 22, 2023 17:14

thisislawatts mentioned this pull request Mar 7, 2023

[feature request] Research contributors get notified about interactions with Research article #2143

Closed

thisislawatts added 2 commits March 7, 2023 18:44

fix: use ternary rather than conditional

d5f0dba

thisislawatts force-pushed the feat/support-contributors-on-research-item branch from 79338e1 to 5a242c1 Compare March 7, 2023 17:45

test: switch to query via testId

5d66df0

thisislawatts force-pushed the feat/support-contributors-on-research-item branch from 5a242c1 to 5d66df0 Compare March 7, 2023 17:46

thisislawatts added 2 commits March 7, 2023 18:58

chore: refactor to move user notifications into store

0f53627

feat: notify contributors #2143

a68d745

chrismclarke self-assigned this Mar 20, 2023

Merge branch 'master' into feat/support-contributors-on-research-item

3a59076

chrismclarke approved these changes Mar 22, 2023

View reviewed changes

chrismclarke merged commit 9d735d2 into master Mar 22, 2023

chrismclarke deleted the feat/support-contributors-on-research-item branch March 22, 2023 13:21

cypress bot mentioned this pull request Mar 22, 2023

🤖 Release PR #2151

Merged

onearmy-bot added the released label Mar 22, 2023

Uh oh!

Conversation

thisislawatts commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Git Issues

Uh oh!

cypress bot commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

4 flaky tests on run #3063 ↗︎

Uh oh!

github-actions bot commented Dec 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisislawatts commented Dec 30, 2022

Uh oh!

davehakkens commented Dec 30, 2022

Uh oh!

davehakkens commented Feb 10, 2023

Uh oh!

thisislawatts commented Feb 11, 2023

Uh oh!

davehakkens commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisislawatts commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thisislawatts commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davehakkens commented Feb 12, 2023

Uh oh!

thisislawatts commented Feb 12, 2023

Uh oh!

davehakkens commented Feb 15, 2023

Uh oh!

davehakkens commented Feb 15, 2023

Uh oh!

davehakkens commented Feb 15, 2023

Uh oh!

thisislawatts commented Feb 16, 2023

Uh oh!

davehakkens commented Feb 16, 2023

Uh oh!

asheerrizvi commented Feb 21, 2023

Uh oh!

chrismclarke left a comment

Choose a reason for hiding this comment

Uh oh!

chrismclarke Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

chrismclarke Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

chrismclarke Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisislawatts Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

chrismclarke Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

chrismclarke Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

davehakkens commented Mar 22, 2023

Uh oh!

onearmy-bot commented Mar 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thisislawatts commented Nov 25, 2022 •

edited

Loading

cypress bot commented Nov 25, 2022 •

edited

Loading

github-actions bot commented Dec 22, 2022 •

edited

Loading

davehakkens commented Feb 11, 2023 •

edited

Loading

thisislawatts commented Feb 11, 2023 •

edited

Loading

thisislawatts commented Feb 12, 2023 •

edited

Loading

chrismclarke Mar 22, 2023 •

edited

Loading