✨ Add pin to top #1267

FelixLgr · 2023-01-15T19:27:48Z

Description

This MR picks up the work already started to finish it. Unfortunately, the idea of the context menu was too difficult in terms of implementation and UI.

Linked issues

Thanks #954 for the help
Close #870

vercel · 2023-01-15T19:27:53Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
gitmoji	✅ Ready (Inspect)	Visit Preview	💬 2 unresolved	Oct 15, 2023 10:18am

packages/website/package.json

vhoyer

I also think we could benefit a lot from having a small hover effect, similar to the one of the gitmoji cards, when we hover over the pin button, this would allow for a nice feedback of the clickable area of the icon

packages/website/src/components/GitmojiList/Gitmoji/index.tsx

packages/website/src/components/GitmojiList/Gitmoji/styles.module.css

vhoyer · 2023-01-16T23:04:57Z

also, you see this area I highlighed in red?

This area has a space of 0.5rem, if we are adding another area to the card, I think it is wise that we apply this space to the grid gap, and not as a padding of the text. Because the icon is way too close to the text there.

looking at the card variation, we should also make the same change, the control of the spacing, shouldn't be managed by the <p/>, but the parent:

carloscuesta

Hey! Added a few comments on the Vercel preview

Thanks for raising the PR! 🙏🏼

FelixLgr · 2023-01-17T23:06:20Z

Hey! Added a few comments on the Vercel preview

Thanks for raising the PR! 🙏🏼

Hey, thanks for the feedback. I'll look into it when I have access.

vhoyer · 2023-01-18T01:01:07Z

I also don't have access to comment or see comments on the vercel preview

carloscuesta · 2023-01-18T19:42:20Z

Oh 😞, looks like I can't give you access since I don't own a team in Vercel, I'll post them here on the PR review

carloscuesta

A couple of things I saw on the PR preview:

I think I would prefer hiding the pin icon by default and make it visible only when the card is hovered or the pin is active
Should we persist pins somewhere? What's the utility of pinning things if they get lost after a page reload?

vhoyer · 2023-01-18T20:46:21Z

2. Should we persist pins somewhere? What's the utility of pinning things if they get lost after a page reload?

Oh, are we not persisting this, it totally passed over my head, I'm in full agreement that we should save those. Probably a localStorage should suffice

vhoyer · 2023-01-18T20:48:19Z

I thought about only showing the icon on hover, but that also is bad UX because it provide no affordance to this action, that's why I suggested that we muted the color a bit so it's less distracting.

FelixLgr · 2023-01-18T21:42:29Z

2. Should we persist pins somewhere? What's the utility of pinning things if they get lost after a page reload?

This is already the case. It's stored in the localStorage (I use useLocalStorage hooks) but it doesn't seem to work (e.g. grid or list display). I think another branch will be needed to fix it.

I looked for it, the value already present doesn't seem to be included and if I try to correct it there seems to be SSR problems (I don't know anything about this subject).
If someone wants to modify the MR it would be with great pleasure

packages/website/src/components/GitmojiList/Gitmoji/styles.module.css

packages/website/src/components/GitmojiList/hooks/useLocalStorage.tsx

carloscuesta · 2023-01-19T19:22:24Z

I thought about only showing the icon on hover, but that also is bad UX because it provide no affordance to this action, that's why I suggested that we muted the color a bit so it's less distracting.

We can try applying a more muted colour, but I think that the hover effect will give a nice "pop" of interactivity instead of showing all empty pins at once (which in my opinion is a bit distracting)

vhoyer · 2023-01-19T22:57:40Z

packages/website/src/components/GitmojiList/index.tsx

+ <ClientOnly>
+ <>
+ {gitmojis.map((gitmoji, index) => (
+ <Gitmoji
+ code={gitmoji.code}
+ description={gitmoji.description}
+ emoji={gitmoji.emoji}
+ isListMode={isListMode}
+ key={index}
+ // @ts-expect-error: This should be replaced with something like:
+ // typeof gitmojis[number]['name'] but JSON can't be exported `as const`
+ name={gitmoji.name}
+ isPinned={isPinned(gitmoji.code)}
+ onPinClick={() => onPinClick(gitmoji.code, gitmoji.emoji)}
+ />
+ ))}
+ </>
+ </ClientOnly>


I don't think that this is particularly good for the experience, I know it was the solution I presented, but this technique should be applied with thought and intention, here it seems like that classical problem "when you only know the hammer, every problem is a nail".

I would refrain from using ClientOnly on the hole list, instead what I would suggest is that we apply the same logic from the ClientOnly to the thing that generated the error.

So in this case, if the sorting based on the localStorage was the source of the problem, let's delay the sorting step to after the page has mounted. That way React can properly hydrate the page (with no differences between server and client) and only after hydration that the sorting is applied. Then we have the best of both worlds:

We have the sorting

We don't have a hydration error

We still have the gitmoji list loaded in the HTML which is good by several reasons.

This same idea could work well on that other problem we have about the grid/list selector.

I tried to implement this in a different way. But the problem is that it makes a laggy effect when the page is loaded (this is especially noticeable if it is the list mode that needs to be loaded)
You can see the effect in the vercel preview

I correct it with a fade effect on the apparition. But I think this is not the best solution.

If you have other ideas I'm interested

let's delay the sorting step to after the page has mounted. That way React can properly hydrate the page (with no differences between server and client) and only after hydration that the sorting is applied.

I find this a bit confusing tbh as you pin an emoji and nothing happens until the page is reloaded 😕

Hey! @FelixLgr perhaps this is something we can improve?

I understand. The current approach prevents favorites from leaving the user's view. I'm open to discussing other solutions if needed.

I've been thinking about this and I was thinking that if every time you add something, it scrolls all the way to the top of the page, I'm not sure it's very pleasant for the user. What do you think?

FelixLgr · 2023-02-04T09:23:32Z

Do you have any additional feedback or changes that I have forgotten?

@vhoyer @carloscuesta

carloscuesta · 2023-02-04T09:51:25Z

Hey! @FelixLgr I'll review soon the PR, one small thing I detected is that the container for the list of emojis is not matching the parent:

See:

packages/website/src/components/ClientOnly/index.tsx

packages/website/src/components/GitmojiList/hooks/useLocalStorage.tsx

packages/website/src/components/GitmojiList/index.tsx

packages/website/src/components/GitmojiList/styles.module.css

carloscuesta · 2023-08-24T06:02:38Z

Hey @carloscuesta & @vhoyer ! Do you still have any specific change requests for my MR? I can't wait to pin the gitmoji. If everything's ok, maybe we can see about the merge?

Hey, do you think we can look at merge? I'll admit that if I don't have to rebase it anymore, I'm happy ahah.

Hey! I'll take a look again

FelixLgr · 2023-10-15T10:16:45Z

Hey, I'm trying to rebase the MR but I got this error on the test :

CarbonAd › should render the component

    TypeError: Cannot read properties of null (reading 'useInsertionEffect')

       5 | describe('CarbonAd', () => {
       6 |   it('should render the component', () => {
    >  7 |     const wrapper = renderer.create(<CarbonAd />)
         |                              ^
       8 |     expect(wrapper).toMatchSnapshot()
       9 |   })
      10 | })

Any ideas ?

chsxf · 2024-02-25T15:57:31Z

I have a question on this PR.

I checked the last Vercel deployment (from 4 months ago) to check how it would work but I don't understand the benefit. Pinning an emoji doesn't bring it to the top of the list / beginning of the grid, even after a refresh.

Is this normal? Is it just for marking emojis are favorites in the list?

(Running Firefox 123.0 on macOS)

FelixLgr · 2024-03-07T19:07:24Z

In my opinion, it's wiser not to bring it back to the top upon clicking the pin, as it could generate a lot of visual interactions. However, since I'm not a UX expert, this is just a personal opinion. On the other hand, the fact that it doesn't happen on page refresh is not normal.

chsxf · 2024-03-16T11:40:40Z

In my opinion, it's wiser not to bring it back to the top upon clicking the pin, as it could generate a lot of visual interactions. However, since I'm not a UX expert, this is just a personal opinion. On the other hand, the fact that it doesn't happen on page refresh is not normal.

Agreed. I could see the benefit if there was some kind of one-click filter to show only pinned gitmojis, But I canno't find one.

FelixLgr commented Jan 15, 2023

View reviewed changes

packages/website/package.json Show resolved Hide resolved

vercel bot deployed to Preview January 15, 2023 19:29 View deployment

vhoyer requested changes Jan 16, 2023

View reviewed changes

carloscuesta reviewed Jan 17, 2023

View reviewed changes

carloscuesta reviewed Jan 18, 2023

View reviewed changes

vercel bot deployed to Preview January 18, 2023 22:49 View deployment

vercel bot deployed to Preview January 19, 2023 10:57 View deployment

vercel bot deployed to Preview January 19, 2023 19:07 View deployment

carloscuesta reviewed Jan 19, 2023

View reviewed changes

vercel bot deployed to Preview January 19, 2023 19:25 View deployment

vercel bot deployed to Preview January 19, 2023 20:24 View deployment

vhoyer reviewed Jan 19, 2023

View reviewed changes

vercel bot deployed to Preview January 21, 2023 00:03 View deployment

vercel bot deployed to Preview January 24, 2023 18:11 View deployment

FelixLgr force-pushed the feature/pin-emoji branch from be9d353 to 23db48e Compare January 24, 2023 18:11

vercel bot deployed to Preview January 24, 2023 18:13 View deployment

carloscuesta requested review from vhoyer and carloscuesta February 4, 2023 09:51

carloscuesta reviewed Feb 4, 2023

View reviewed changes

FelixLgr and others added 22 commits October 15, 2023 11:39

✨ Add pin to top

6c925ff

💄 Update UI after MR feedback

6b60768

🏷️ Avoid using Array<> Generic in favor of [] notation

8a67f41

✨ Add ClientOnly component

30f71c1

🐛 Fix difference between server and client render

05a9572

💄 Increase the clickable area of the pin button

bade345

♻️ Refacto useLocalStorage hooks

d00c182

🐛 Fix difference between server and client render

0c25677

✨ Update ClientOnly component for test

1f12205

🏷️ Update types of useLocalStorage hooks

7793764

💄 Remove negative margin for the pin button

52d5752

🔥 Remove unnecessary import

0d2fd9c

♻️ Refacto SSR mounted test

aca1837

💄 Added a fade-in animation to avoid the appearance of flash.

c548a5b

➕ Add usehooks-ts

12cf42a

🏷️ Update ClientOnly props type

2aa0713

💄 🐛 Update style

097bac0

♻️ Refacto after feedback

708b418

📸 Update tests

dd18173

🚨 Fix lint

29e91d7

♻️ Migrate from yarn to pnpm

e1168aa

🔥 Remove ClientOnly component

3d84643

🚨 Fix lint

df6f37b

FelixLgr force-pushed the feature/pin-emoji branch from 38dccc0 to df6f37b Compare October 15, 2023 10:18

vercel bot deployed to Preview October 15, 2023 10:18 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add pin to top #1267

✨ Add pin to top #1267

FelixLgr commented Jan 15, 2023

vercel bot commented Jan 15, 2023 •

edited

vhoyer left a comment

vhoyer commented Jan 16, 2023

carloscuesta left a comment

FelixLgr commented Jan 17, 2023

vhoyer commented Jan 18, 2023

carloscuesta commented Jan 18, 2023

carloscuesta left a comment

vhoyer commented Jan 18, 2023

vhoyer commented Jan 18, 2023

FelixLgr commented Jan 18, 2023 •

edited

carloscuesta commented Jan 19, 2023

vhoyer Jan 19, 2023

FelixLgr Jan 21, 2023

FelixLgr Jan 24, 2023

carloscuesta Feb 7, 2023

carloscuesta Aug 25, 2023

FelixLgr Sep 3, 2023

FelixLgr Oct 15, 2023

FelixLgr commented Feb 4, 2023

carloscuesta commented Feb 4, 2023

carloscuesta commented Aug 24, 2023

FelixLgr commented Oct 15, 2023

chsxf commented Feb 25, 2024

FelixLgr commented Mar 7, 2024

chsxf commented Mar 16, 2024

✨ Add pin to top #1267

Are you sure you want to change the base?

✨ Add pin to top #1267

Conversation

FelixLgr commented Jan 15, 2023

Description

Linked issues

vercel bot commented Jan 15, 2023 • edited

vhoyer left a comment

Choose a reason for hiding this comment

vhoyer commented Jan 16, 2023

carloscuesta left a comment

Choose a reason for hiding this comment

FelixLgr commented Jan 17, 2023

vhoyer commented Jan 18, 2023

carloscuesta commented Jan 18, 2023

carloscuesta left a comment

Choose a reason for hiding this comment

vhoyer commented Jan 18, 2023

vhoyer commented Jan 18, 2023

FelixLgr commented Jan 18, 2023 • edited

carloscuesta commented Jan 19, 2023

vhoyer Jan 19, 2023

Choose a reason for hiding this comment

FelixLgr Jan 21, 2023

Choose a reason for hiding this comment

FelixLgr Jan 24, 2023

Choose a reason for hiding this comment

carloscuesta Feb 7, 2023

Choose a reason for hiding this comment

carloscuesta Aug 25, 2023

Choose a reason for hiding this comment

FelixLgr Sep 3, 2023

Choose a reason for hiding this comment

FelixLgr Oct 15, 2023

Choose a reason for hiding this comment

FelixLgr commented Feb 4, 2023

carloscuesta commented Feb 4, 2023

carloscuesta commented Aug 24, 2023

FelixLgr commented Oct 15, 2023

chsxf commented Feb 25, 2024

FelixLgr commented Mar 7, 2024

chsxf commented Mar 16, 2024

vercel bot commented Jan 15, 2023 •

edited

FelixLgr commented Jan 18, 2023 •

edited