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.

Sign up for GitHub

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

Jump to bottom

fix: locking demo bugs #146

Merged
merged 2 commits into from
Aug 30, 2023
Merged

fix: locking demo bugs #146

merged 2 commits into from
Aug 30, 2023

Conversation

ttypic
Copy link
Collaborator

@ttypic ttypic commented Aug 30, 2023

  • The 'click elsewhere to lose selection' applied to locations as well, not just locks
  • When more than one person is on the same component (the pictures that are not locked), the location label say "... +N".
  • When someone leaves, the avatar change to grey
  • Sanitize value from useChannel

@ttypic
fix: locking demo bugs
1e4fe9a 
* The 'click elsewhere to lose selection' applied to locations as well, not just locks
* When more than one person is on the same component (the pictures that are not locked), the location label say "... +N".
* When someone leaves, the avatar change to grey
* Sanitize value from `useChannel`
@ttypic ttypic requested a review from dpiatek August 30, 2023 07:54
dpiatek
dpiatek reviewed Aug 30, 2023
View reviewed changes
demo/src/components/Image.tsx Outdated
const memberName = getMemberFirstName(activeMember);
const label = self?.connectionId === activeMember?.connectionId ? 'You' : memberName;
const activeMembers = findActiveMembers(id, slide, members);
const locatedByMe = activeMembers.some((member) => member.connectionId === self?.connectionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

small thing, but perhaps instead of located we should use the word occupied ?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I thought at first located was a typo for locked!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

totally agree, you are absolutely right!

dpiatek
dpiatek approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@dpiatek dpiatek left a comment

Choose a reason for hiding this comment

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

great stuff 👍🏻

@ttypic ttypic merged commit 63d1a5f into main Aug 30, 2023
5 checks passed
@ttypic ttypic deleted the locking-demo-improvements branch August 30, 2023 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpiatek dpiatek dpiatek approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ttypic @dpiatek