:sparkles: Add pin to top

[FelixLgr]

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[bot]]

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

[vhoyer]

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:

[FelixLgr]

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]

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

[carloscuesta]

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

[vhoyer]

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]

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]

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

[carloscuesta]

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)

[FelixLgr]

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

@vhoyer @carloscuesta

[carloscuesta]

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:

[FelixLgr]

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?

[FelixLgr]

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.

[carloscuesta]

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]

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]

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]

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]

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.