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(Reactions): added new Reactions component #197

Merged
merged 28 commits into from
Aug 20, 2024

Conversation

Ruminat
Copy link
Contributor

@Ruminat Ruminat commented May 31, 2024

Reactions component

A component for managing users' reactions:

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

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

Preview is ready.

src/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
src/components/Reactions/Reactions.scss Outdated Show resolved Hide resolved
@Ruminat
Copy link
Contributor Author

Ruminat commented Jun 6, 2024

@ogonkov @korvin89 @amje @ValeraS ping

@amje
Copy link
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.

src/components/Reactions/mock/mockData.ts Outdated Show resolved Hide resolved
src/components/Reactions/Reactions.tsx Outdated Show resolved Hide resolved
@Ruminat
Copy link
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
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
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
Contributor Author

Ruminat commented Jun 14, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Contributor Author

Ruminat commented Jun 17, 2024

@amje ping

@amje
Copy link
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
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
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
Contributor Author

Ruminat commented Jul 17, 2024

@amje ping

@amje
Copy link
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.

src/components/Reactions/hooks.ts Outdated Show resolved Hide resolved
@Ruminat
Copy link
Contributor Author

Ruminat commented Jul 19, 2024

@amje I implemented your suggested API

@Ruminat
Copy link
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
Contributor Author

Ruminat commented Jul 24, 2024

I also added myself as owner of Notifications and Reactions components

@Ruminat
Copy link
Contributor Author

Ruminat commented Jul 25, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Contributor Author

Ruminat commented Jul 31, 2024

@amje ping

src/components/Reactions/Reactions.tsx Outdated Show resolved Hide resolved
src/components/Reactions/Reactions.tsx Show resolved Hide resolved
src/components/Reactions/Reactions.scss Outdated Show resolved Hide resolved
src/components/Reactions/Reactions.scss Outdated Show resolved Hide resolved
src/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
src/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
src/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
src/components/Reactions/Reaction.tsx Outdated Show resolved Hide resolved
@Ruminat
Copy link
Contributor Author

Ruminat commented Aug 8, 2024

@amje ping

1 similar comment
@Ruminat
Copy link
Contributor Author

Ruminat commented Aug 9, 2024

@amje ping

@Ruminat
Copy link
Contributor Author

Ruminat commented Aug 20, 2024

@amje ping

@Ruminat Ruminat merged commit be23d4e into main Aug 20, 2024
4 checks passed
@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