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(presence-tracker): Add "reactions" using presence notifications #23294

Open
wants to merge 102 commits into
base: main
Choose a base branch
from

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Dec 11, 2024

Summary of changes

The presence-tracker example now uses the notifications infrastructure from the presence package to send "reactions" to other clients. Users can select any emoji from an emoji picker, and any left click in the window will send the reaction to other users. Other clients will see the reaction animate and fade out.

Implementation notes

  • Unlike the mouse and focus trackers, I did not create a class to manage things. Instead, I just wrote an init function that initializes the reactions for the whole app. I think this makes it a little easier to see how things fit together, but could be refactored if needed.
  • The emoji picker element has a bug with node16 module resolution. The fix is straightforward so I just patched the local dep. Upstream fix PR is already opened.
  • I considered a smaller/more limited reaction selection UX, but the emoji picker was easy to adopt, so I went with that.

Known issues

  • TODOs to check connection status before sending.
  • No testing - not sure how to do it or if it's worth it since we have tests on notifications themselves already.

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170508 links
    1603 destination URLs
    1842 URLs ignored
       0 warnings
       0 errors


@github-actions github-actions bot removed the dependencies Pull requests that update a dependency file label Jan 23, 2025
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 24, 2025
examples/apps/presence-tracker/src/app.ts Outdated Show resolved Hide resolved
examples/apps/presence-tracker/src/reactions.ts Outdated Show resolved Hide resolved
examples/apps/presence-tracker/src/reactions.ts Outdated Show resolved Hide resolved
@tylerbutler tylerbutler marked this pull request as ready for review January 25, 2025 00:13
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

A couple of small things, otherwise LGTM. Disclaimer: I might not be the target audience you were interested in for a "new eyes" review, I've dealt a bit with the presence framework before for the ai-collab example app.

Copy link
Contributor

Choose a reason for hiding this comment

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

The fix was merged and released yesterday, no more need for a patch 🥳

// In the future, we'll be able to use IMousePosition here.
position: { x: number; y: number },
value: string,
intensity: "normal" | "intense",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to exercise the "intense" case? If not I'd say remove it to keep things simple.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants