-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
✨ Add pin to top #1267
base: master
Are you sure you want to change the base?
✨ Add pin to top #1267
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/styles.module.css
Outdated
Show resolved
Hide resolved
packages/website/src/components/GitmojiList/Gitmoji/styles.module.css
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Added a few comments on the Vercel preview
Thanks for raising the PR! 🙏🏼
I also don't have access to comment or see comments on the vercel preview |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Oh, are we not persisting this, it totally passed over my head, I'm in full agreement that we should save those. Probably a |
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. |
This is already the case. It's stored in the 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). |
packages/website/src/components/GitmojiList/Gitmoji/styles.module.css
Outdated
Show resolved
Hide resolved
packages/website/src/components/GitmojiList/Gitmoji/styles.module.css
Outdated
Show resolved
Hide resolved
packages/website/src/components/GitmojiList/hooks/useLocalStorage.tsx
Outdated
Show resolved
Hide resolved
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) |
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! @FelixLgr perhaps this is something we can improve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. The current approach prevents favorites from leaving the user's view. I'm open to discussing other solutions if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
be9d353
to
23db48e
Compare
Do you have any additional feedback or changes that I have forgotten? |
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/GitmojiList/hooks/useLocalStorage.tsx
Outdated
Show resolved
Hide resolved
Hey! I'll take a look again |
Hey, I'm trying to rebase the MR but I got this error on the test :
Any ideas ? |
38dccc0
to
df6f37b
Compare
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) |
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. |
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