Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(comments): implement reactions #5409

Merged
merged 9 commits into from
Jan 19, 2024
Merged

feat(comments): implement reactions #5409

merged 9 commits into from
Jan 19, 2024

Conversation

hermanwikner
Copy link
Member

@hermanwikner hermanwikner commented Dec 20, 2023

Description

This pull request introduces reactions to the comments feature. In the first iteration, there will be a fixed set of reactions to select from, however, in future iterations, we may introduce the complete palette of reaction options. The reactions are stored in the comment document in the 'reactions' array. Each individual reaction is an object with the following properties:

  • _key - A unique key for the reaction. The key is constructed by combining the user ID and the short name of the reaction.
  • shortName - The short name of the reaction. The short names follow the naming convention listed here.
  • userId - The ID of the user who added the reaction.
  • addedAt - The date when the reaction was added.
reactions

Why is the _optimisticState needed?

This flag is required to ensure that reactions removed optimistically but not yet processed on the server are not displayed. This scenario arises when a user removes Reaction A and, while the API request for that removal is still processing, removes Reaction B. There is a chance that Reaction B might temporarily reappear in the UI when the updated list of reactions is received. This is because the updated list, received after the removal of Reaction A, may not yet reflect the removal of Reaction B. Consequently, Reaction B might temporarily reappear in the UI until the updated list, including the removal of Reaction B, is received.

To address this issue, we utilize the _optimisticState flag to track the removal or addition of reactions locally. When the server responds with an updated list of reactions, we filter out reactions marked with the _optimisticState set to "removed." This prevents reactions that were removed optimistically but are still being processed from reappearing in the UI.

What to review

  • Make sure that the feature works as expected.
  • Make sure that the data model is solid and future-proof.
  • Make sure that the implementation is accessible.
  • Test the resilience by intentionally removing and adding reactions frequently.

Notes for release

Make it possible to react to comments

@hermanwikner hermanwikner changed the title feat(comments): implement reactions feat(comments): implement reactions [WIP] Dec 20, 2023
Copy link

vercel bot commented Dec 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Jan 19, 2024 3:53pm
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 19, 2024 3:53pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Jan 19, 2024 3:53pm

Copy link
Contributor

github-actions bot commented Dec 20, 2023

Package Documentation Change
sanity -3%
Full Report
@sanity/diff
This branch Next branch
13 documented 13 documented
16 not documented 16 not documented
@sanity/block-tools
This branch Next branch
4 documented 4 documented
9 not documented 9 not documented
@sanity/types
This branch Next branch
55 documented 55 documented
234 not documented 234 not documented
sanity/desk
This branch Next branch
84 documented 84 documented
64 not documented 64 not documented
@sanity/portable-text-editor
This branch Next branch
21 documented 21 documented
44 not documented 44 not documented
@sanity/mutator
This branch Next branch
7 documented 7 documented
4 not documented 4 not documented
@sanity/cli
This branch Next branch
1 documented 1 documented
31 not documented 31 not documented
@sanity/schema/_internal
This branch Next branch
0 documented 0 documented
12 not documented 12 not documented
@sanity/util/paths
This branch Next branch
1 documented 1 documented
15 not documented 15 not documented
sanity/router
This branch Next branch
17 documented 17 documented
26 not documented 26 not documented
@sanity/util/legacyDateFormat
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/schema
This branch Next branch
0 documented 0 documented
2 not documented 2 not documented
sanity/structure
This branch Next branch
2 documented 2 documented
6 not documented 6 not documented
sanity/cli
This branch Next branch
2 documented 2 documented
0 not documented 0 not documented
@sanity/vision
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/util/fs
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
sanity/_internal
This branch Next branch
0 documented 0 documented
1 not documented 1 not documented
sanity/_internalBrowser
This branch Next branch
0 documented 0 documented
3 not documented 3 not documented
@sanity/util/content
This branch Next branch
1 documented 1 documented
5 not documented 5 not documented
sanity
This branch Next branch
177 documented 183 documented
843 not documented 846 not documented

Copy link
Contributor

github-actions bot commented Dec 20, 2023

Component Testing Report Updated Jan 19, 2024 3:52 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 33s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 12s 3 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 13s 4 2 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 34s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 18s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 2s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 13s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 7s 3 0 0

Copy link
Contributor

@pedrobonamin pedrobonamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Thanks @hermanwikner
Left a suggestion and some questions, but none are blockers.

Copy link
Contributor

@robinpyon robinpyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work @hermanwikner!

@hermanwikner hermanwikner added this pull request to the merge queue Jan 19, 2024
Merged via the queue into next with commit 8c140be Jan 19, 2024
40 checks passed
@hermanwikner hermanwikner deleted the edx-659 branch January 19, 2024 15:55
juice49 pushed a commit that referenced this pull request Jan 22, 2024
* feat(comments): add reaction components

* dev(test-studio): add comment reactions workshop stories

* feat(comments): comment reaction operation and data handling

* feat(comments): add reactions to comments plugin

* test(comments): utility function for merging two lists of comments

* feat(comments): pass `readOnly` value to reactions

* refactor(comments): simplify reaction users list

* fix: force emoji-specific fonts, prefer buttons over cards, minor styling updates (#5524)

* fix(comments): use `TooltipDelayGroupProvider` from ui-components

---------

Co-authored-by: Robin Pyon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants