Skip to content

feat(Reactions): added new Reactions component#197

Merged
Ruminat merged 28 commits intomainfrom
feature/reactions-component
Aug 20, 2024
Merged

feat(Reactions): added new Reactions component#197
Ruminat merged 28 commits intomainfrom
feature/reactions-component

Conversation

@Ruminat
Copy link
Copy Markdown
Contributor

@Ruminat Ruminat commented May 31, 2024

Reactions component

A component for managing users' reactions:

изображение изображение

@Ruminat Ruminat requested review from ValeraS, amje and korvin89 as code owners May 31, 2024 13:15
@gravity-ui-bot
Copy link
Copy Markdown
Contributor

Preview is ready.

Comment thread src/components/Reactions/Reaction.tsx Outdated
Comment thread src/components/Reactions/Reactions.scss Outdated
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 6, 2024

@ogonkov @korvin89 @amje @ValeraS ping

@amje
Copy link
Copy Markdown
Contributor

amje commented Jun 6, 2024

@Ruminat Let's simplify the API, currently it looks a bit hard to understand and it stores the state in multiple places (reactions and palette.options). What do you think about this API suggestion?

// Do not extend whole PaletteOption, only pick needed props. This makes refactor easier
interface ReactionsItem extends Pick<PaletteOption, 'value' | 'content' | 'title'> {
    count: number;
    selected?: boolean;
}

interface ReactionsProps {
    // This would map into Palette props (options and value depending on selected states) and create buttons if count > 0
    reactions: ReactionsItem[];
    // User should handle this callback and change reactions array
    onToggle?: (reaction: ReactionsItem) => void;
    // Extract tooltip render to dedicated prop and pass whole reaction item
    // No other tooltip props needed for the sake of simplicty
    renderTooltipContent?: (reaction: ReactionsItem) => React.ReactNode;
    // Keep only columns. Other Palette props are useless
    columns?: number;
    // Rest props
    // ...
}

The only source of state is now the reactions array, which user have to update on onToggle calls. I also extracted tooltip prop of reaction into top-level prop renderTooltipContent. Copying the same tooltip props for each reaction is redundant.

Comment thread src/components/Reactions/mock/mockData.ts
Comment thread src/components/Reactions/Reactions.tsx Outdated
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 7, 2024

@Ruminat Let's simplify the API, currently it looks a bit hard to understand and it stores the state in multiple places (reactions and palette.options). What do you think about this API suggestion?

// Do not extend whole PaletteOption, only pick needed props. This makes refactor easier
interface ReactionsItem extends Pick<PaletteOption, 'value' | 'content' | 'title'> {
    count: number;
    selected?: boolean;
}

interface ReactionsProps {
    // This would map into Palette props (options and value depending on selected states) and create buttons if count > 0
    reactions: ReactionsItem[];
    // User should handle this callback and change reactions array
    onToggle?: (reaction: ReactionsItem) => void;
    // Extract tooltip render to dedicated prop and pass whole reaction item
    // No other tooltip props needed for the sake of simplicty
    renderTooltipContent?: (reaction: ReactionsItem) => React.ReactNode;
    // Keep only columns. Other Palette props are useless
    columns?: number;
    // Rest props
    // ...
}

The only source of state is now the reactions array, which user have to update on onToggle calls. I also extracted tooltip prop of reaction into top-level prop renderTooltipContent. Copying the same tooltip props for each reaction is redundant.

Yeah, I like that solution, I'll change the code accordingly

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 7, 2024

@amje I started implementing your solution and encountered a problem:

let's say you have three reactions: 😊, 👍 and 😢
and at first you have this state: 😊:0 👍:0 😢:0 and the user sees no reactions
the user clicks on 😢, so the state becomes 😊:0 👍:0 😢:1, the users sees 😢:1
and when the users clicks on 😊, instead of seeing 😢:1 😊:1 he sees 😊:1 😢:1., because we just increase a counter and don't change the emoji's position
you can change the position though, so the user sees 😢:1 😊:1, but in such a case Palette's order of options will change accordingly

So I guess we have to use 2 states here

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 7, 2024

I've also noticed this comment:

// Extract tooltip render to dedicated prop and pass whole reaction item
// No other tooltip props needed for the sake of simplicty

Tooltip is a little bit harder than just rendering content, tooltip has the following type:

export interface ReactionTooltipProps
    extends Pick<PopoverProps, 'strategy' | 'placement' | 'modifiers'> {
    /**
     * Tooltip's content.
     */
    content: React.ReactNode;
    /**
     * Tooltip content's HTML class attribute.
     */
    className?: string;
    /**
     * Fires when the `onMouseLeave` callback is called.
     * Usage example:
     * you have some popup inside a tooltip, you hover on it, you don't want the tooltip to be closed because of that.
     */
    canClosePopup?: () => boolean;
}

className might not be important, but canClosePopup can be. In Arcanum, for example, it is used to block the tooltip's closure on hovering on a user card popup. Pick<PopoverProps, 'strategy' | 'placement' | 'modifiers'> can be important as well because uikit's Popups can act strange sometimes. For example, in Nirvana we had to hack it a bit so it doesn't go over the visible part of the screen.

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 14, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jun 17, 2024

@amje ping

@amje
Copy link
Copy Markdown
Contributor

amje commented Jun 19, 2024

@amje I started implementing your solution and encountered a problem:

let's say you have three reactions: 😊, 👍 and 😢 and at first you have this state: 😊:0 👍:0 😢:0 and the user sees no reactions the user clicks on 😢, so the state becomes 😊:0 👍:0 😢:1, the users sees 😢:1 and when the users clicks on 😊, instead of seeing 😢:1 😊:1 he sees 😊:1 😢:1., because we just increase a counter and don't change the emoji's position you can change the position though, so the user sees 😢:1 😊:1, but in such a case Palette's order of options will change accordingly

So I guess we have to use 2 states here

@Ruminat Don't see a problem here. Having predefined order is good. GitHub reactions work exactly like so

@amje
Copy link
Copy Markdown
Contributor

amje commented Jun 19, 2024

I've also noticed this comment:

// Extract tooltip render to dedicated prop and pass whole reaction item
// No other tooltip props needed for the sake of simplicty

Tooltip is a little bit harder than just rendering content, tooltip has the following type:

export interface ReactionTooltipProps
    extends Pick<PopoverProps, 'strategy' | 'placement' | 'modifiers'> {
    /**
     * Tooltip's content.
     */
    content: React.ReactNode;
    /**
     * Tooltip content's HTML class attribute.
     */
    className?: string;
    /**
     * Fires when the `onMouseLeave` callback is called.
     * Usage example:
     * you have some popup inside a tooltip, you hover on it, you don't want the tooltip to be closed because of that.
     */
    canClosePopup?: () => boolean;
}

className might not be important, but canClosePopup can be. In Arcanum, for example, it is used to block the tooltip's closure on hovering on a user card popup. Pick<PopoverProps, 'strategy' | 'placement' | 'modifiers'> can be important as well because uikit's Popups can act strange sometimes. For example, in Nirvana we had to hack it a bit so it doesn't go over the visible part of the screen.

You are trying to fix those issues implementing extra stuff in the component that not needed. It's better to have simple and robust API that would work predictable.

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 11, 2024

@amje I simplified ReactionProps's tooltip to simply React.ReactNode and removed content and title props from ReactionProps (moved content to ReactionInnerProps, so it's calculated in the Reactions component, and got rid of the title completely)

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 17, 2024

@amje ping

@amje
Copy link
Copy Markdown
Contributor

amje commented Jul 18, 2024

@Ruminat As we discussed eariler, let's implement the following API:

interface Reaction {
  value: string; // Unique identifier of reaction
  content: React.ReactNode; // Reaction's content
  title?: string; // Alternative content via browser's tooltip
}

interface ReactionState {
  value: string;
  selected?: boolean;
  count?: number;
}

interface ReactionsProps extends QAProps, DOMProps {
  reactions: Reaction[]; // <-- Keep all data in single array
  reactionsState?: ReactionState[]; // <-- Keep all state in place
  onToggle?: (value: string) => void; // User should update reactionsState in this callback
  // Other props
  readOnly?: boolean; // Instead of disabling buttons and coloring them to gray we hide palette trigger and ignore hover/click events
  paletteProps?: Pick<PaletteProps, 'columns' | 'rowClassName' | 'optionClassName'> // Only pick props related to view
}

It's robust, simple and easy to understand.

Comment thread src/components/Reactions/hooks.ts Outdated
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 19, 2024

@amje I implemented your suggested API

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 23, 2024

@amje I also added tooltipBehavior property to Reactions component:
It defined how a reaction's tooltip should act:

  1. as a tooltip (you can't hover over the contents — it disappeares),
  2. as a popover (you can hover over the contents — it doesn't disappear).

What do you think?

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 24, 2024

I also added myself as owner of Notifications and Reactions components

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 25, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Jul 31, 2024

@amje ping

Comment thread src/components/Reactions/Reactions.tsx Outdated
Comment thread src/components/Reactions/Reactions.tsx
Comment thread src/components/Reactions/Reactions.scss Outdated
Comment thread src/components/Reactions/Reactions.scss Outdated
Comment thread src/components/Reactions/Reaction.tsx Outdated
Comment thread src/components/Reactions/Reaction.tsx Outdated
Comment thread src/components/Reactions/Reaction.tsx Outdated
Comment thread src/components/Reactions/Reaction.tsx Outdated
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Aug 8, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Aug 9, 2024

@amje ping

@Ruminat
Copy link
Copy Markdown
Contributor Author

Ruminat commented Aug 20, 2024

@amje ping

@Ruminat Ruminat merged commit be23d4e into main Aug 20, 2024
@Ruminat Ruminat deleted the feature/reactions-component branch August 20, 2024 13:55
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.

4 participants