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

Show richer zap reactions #547 #557

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

ticruz38
Copy link
Collaborator

In the notification tab:

If there is more than one zap, show all zap activity and aggregate similar messages (many are boilerplate) into chips containing the message and aggregated people's avatar.
Same thing for reactions.
Screenshot 2025-01-29 at 16 48 16

If it is a one person action, show the zap amount and content as well, but also the date of the event

Screenshot 2025-01-29 at 17 05 09

Most of the logic happens in the NoteActivity component, it handles reactions and interactions and differ quite a bit from NoteMeta.

@ticruz38 ticruz38 changed the base branch from master to dev January 29, 2025 16:26
@staab
Copy link
Collaborator

staab commented Jan 29, 2025

  • Try to reduce the redundant layout code (date should always be in the same place, at the same font size
  • Zaps aren't correct, you're using the same zapper for all zaps, even from different people
  • Given that interactions/replies kinds share very little code, go ahead and separate the two
  • Remove borders and make the border radius for the pills rounded-full
  • Use the same pill style for zaps by multiple people as for reactions
  • Make pills clickable, opening a modal with the list of people in it (see PeopleAction for some code to copy)

@ticruz38
Copy link
Collaborator Author

ticruz38 commented Jan 30, 2025

Zaps aren't correct, you're using the same zapper for all zaps, even from different people

It's a copy/paste from zaps in NoteActions. My understanding is that all events (here context) zapping the same root event have the same zapper (the root event zapper). It seems to be working well, but my test surface for zaps is low.

Try to reduce the redundant layout code (date should always be in the same place, at the same font size

I could not reuse exactly the same layout as in NotificationItem for NoteReactions. It is a multiline component, so one different date and different formatting for zap and reaction. The code could be reused between zap and reactions but that would involve a bunch of slot and/or props, for me it is not worth it, I can still introduce another component if needed.
Dates used to be wrapped in a small tag, it is the case in NoteReactions and NoteInteractions too

A few more screenshots

Screenshot 2025-01-30 at 13 11 19 Screenshot 2025-01-30 at 13 11 40

Copy link
Collaborator

@staab staab left a comment

Choose a reason for hiding this comment

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

  • Give the pills a little margin (probably px-2 py-1, or maybe just p-1)
  • Only show a single date for each group of reactions/zaps/reposts
  • Use text-sm to make the description text the same size as the date

src/app/shared/NotificationItem.svelte Outdated Show resolved Hide resolved
@ticruz38
Copy link
Collaborator Author

Implemented the suggested changes, the date looks out of place in certain conditions, here when multiple zaps are in, it shrink the zap container, while anything below (here the likes) will take the full width. There is no easy "fix" for this

Screenshot 2025-01-31 at 13 12 08

Copy link
Collaborator

@staab staab left a comment

Choose a reason for hiding this comment

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

I agree, let's just remove the date entirely since we already have the day showing above.

</div>
<NoteReducer shouldAwait shouldSort {events} depth={1} bind:items let:event let:getContext>
<slot {event} context={getContext(event)}>
<div>Missing child component</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for an error, just use a self-closing slot tag

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.

2 participants