Feature/pin to top #954
Feature/pin to top #954
Conversation
This reverts commit 1b8dd28.
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/carloscuesta/gitmoji/4JMN4yzNrV3qDcYdYw9YyLAyDPh6 |
|
pink pins have bad visibility on grid view, we should probably paint them white in this case.
on list view the pins overlap with some large texts I also don't quite like this pin svg, don't we have a simpler maybe smaller one? also the drop-shadow around it makes it a little too "information heavy" for my tastes. |
|
@vhoyer I agree that this icon is kind of 'meeh' at the moment but didn't found at a better UI yet. Wasn't also sure what's the best between a star or a pin. |
|
Hey! Thanks for the effort I will review this by the end of the day 😊 |
| className={` | ||
| ${styles.emoji} col-xs-12 col-sm-6 | ||
| ${props.isListMode ? 'col-md-4' : 'col-md-3'} | ||
| ${props.isListMode ? styles.list : ''} | ||
| `} |
There was a problem hiding this comment.
have we thought about using classnames cuz this is getting kinda ridiculous haha
There was a problem hiding this comment.
Agree let's use classnames to simplify this
There was a problem hiding this comment.
should @johannchopin install it and make use of it here (and then we create another PR to refactor the rest of the app), or should we do a separate PR including the classnames and refactoring the hole project?
There was a problem hiding this comment.
@vhoyer would be preferable to do so in another PR when this one is done 👍
where did you find this svg icon, @johannchopin? on a side note: @carloscuesta don't you have an icon pack that you like to use? |
johannchopin
force-pushed
the
feature/pin-to-top
branch
from
63191a9
to
a3255f5
Compare
|
@vhoyer The icons are comming from the bootstrap free collection --> https://icons.getbootstrap.com/ |
Not really 🤣 The icon placement seems a bit confusing to me, in grid mode is on the header but in list appears on the card:
I would suggest to keep the icon in the same place no matter what the view mode is |
|
For the button I would suggest doing a container on top of the icon as this will help us avoiding a11y issues with the contrast of the multiple colors that we have Something like this:
|
@carloscuesta It's common to see actions at the top left of an element like:
rather than this layout that is really confusing for me (the 2 buttons are to close):
Also moving the button in the header will reduce the emojis space. |
|
The only problem I see with the left positioning approach is that the |
@johannchopin @carloscuesta how about we make a proper column for the pin button, like: this way we can have the button at the more intuitive corner, and we also don't overlap the text. If the empty space is a concern, we could just float the pinButton to the right, that way the pinButton wouldn't overlap with the text, and the text would wrap around the button. |
|
Hey @vhoyer I don't understand what you mean with the "code representation" perhaps you can mock it with an image? |
|
oh, the notation is basically css' |
|
@carloscuesta @vhoyer another approach would be to move the button to the bottom right:
|
|
Hey! This PR has been stall for a while so I'll close it! Feel free to reopen it 🙏🏼 |
Description
Closes #870.
@carloscuesta @vhoyer Here is my implementation of pinning a gitmoji to the top of the page. It only store the list of codes in the localStorage and sort them accordingly.
Tests