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

WIP: ✨ Add raw layout to gitmojis list #453

Conversation

johannchopin
Copy link
Collaborator

Description

Feature where you can toggle the gitmojis list between a raw and grid layout. So something like that:

gitmoji

Closes #314.

Tests

  • All tests passed.

@johannchopin johannchopin added the improvement Website improvements label Jun 9, 2020
@johannchopin
Copy link
Collaborator Author

Quite nice with a custom toggle layout button:

gitmoji

@johannchopin
Copy link
Collaborator Author

johannchopin commented Jun 13, 2020

Hey @carloscuesta, @vhoyer I wanted to have your opinion about this implementation of the feature before continuing. So I will love to have some feedback 😊

@vhoyer
Copy link
Collaborator

vhoyer commented Jun 14, 2020

So, I like the idea, here are some considerations (while I didn't look at your code yet):

  1. I didn't like that the mobile raw view hide the description, maybe we could do something like this:
/* I'm not suggesting you use grid layout, I'm just using it to describe my layout suggestion */
grid-template: 
  'icon title'       auto
  'icon description' auto
  /auto 1fr;
  1. I don't like the name "raw" maybe we could change it to "list view" vs "grid view" is better and more widely used.
  2. Do you know when we are reviewing a PR on github, there is the tab for files? There, we can choose between "unified" view and "split" view. When you have "unified" selected, the file diff has a fixed width, that is to be easier to read (basic UX there), but when you switch to "split" view, you will have the file expand horizontally until the end of your screen:
    Screenshot showing file changed tab with the two modes, the unified with a fixed width and the slit being horizontally fluid, that is, expanding itself to fill the screen
    I think we can utilize the same strategy for this PR, having the list view being horizontally fluid, and having the grid view being fixed.
  3. I'd go for a desgin more like this, tell me what you think 😄 :
    image

so this is my feedback 😄 sorry for being so extensive, but there is a lot to discuss :D

@vhoyer
Copy link
Collaborator

vhoyer commented Jun 15, 2020

Tho, I'm quite bad at designing things ➖ :feelsgood:

😅

@johannchopin
Copy link
Collaborator Author

@vhoyer Ok so make some updates concerning topics 1 and 2 which makes totally sense 👍 Not sure about topic 4, I prefer to have an icon that illustrate the layout since it's used by most of the interfaces (Google, Font-Awesome). But it's probably better to not have a toggle button and instead this type of UI like Font-Awesome has:

image

What do you think about it?

I agree with 3 but need for that to change aspect of <main> which is in the parent of GitmojiList component. So I will try it and see if the changes are not to 'ugly' in a code POV

@vhoyer
Copy link
Collaborator

vhoyer commented Jun 18, 2020

What do you think about it?

I think you are right.

I agree with 3 but need for that to change aspect of

which is in the parent of GitmojiList component. So I will try it and see if the changes are not to 'ugly' in a code POV

I think we can figure a way of doing it without being ugly :D, let's try, if you have trouble, let me know :D

@johannchopin johannchopin force-pushed the feature/raw-mode-of-GitmojisList branch 2 times, most recently from 1db8b27 to 39c5b7f Compare June 27, 2020 14:44
@johannchopin
Copy link
Collaborator Author

Make some changes according to #453 (comment)

What do you think about it @vhoyer, @carloscuesta?

gitmoji

@johannchopin johannchopin force-pushed the feature/raw-mode-of-GitmojisList branch 2 times, most recently from cfa15eb to a9fdea6 Compare June 27, 2020 15:23
@vhoyer
Copy link
Collaborator

vhoyer commented Jun 29, 2020

problems:

  1. apperently the default width for the grid has changed, which made the cards feel a bit funky:
    on master: (I think it's the display flex on the div.wrap)
    image
    on this branch:
    image

  2. I didn't like that the icons for switching the view are aligned to the screen end, I'd like then to be aligned with the container
    On this branch:
    image
    What I'd like to see:
    image

  3. Tablet(768px) view is... a little bit strange:
    image

  4. I think we could benefit from a little bit (like .25em) of padding (top/bottom) on #gitmoji-list .emoji-raw .emoji-info

@vercel
Copy link

vercel bot commented Jun 29, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carloscuesta/gitmoji/5yfzzimls
✅ Preview: https://gitmoji-git-57bc8d8e673eeb8c950d0e65d15296e24ab9bf2f.carloscuesta.vercel.app

@johannchopin
Copy link
Collaborator Author

johannchopin commented Jul 4, 2020

Hey @vhoyer 👋 Sorry for late answer 😅

Totally agree with point 1, 3, 4 I will fix it 👍

For point 2 I'm not sure of the design to implement 🤔 Having the icons a little bit in the center make the raw layout feel strange 🤔 Or maybe it's not so gross. What do you think? 😄

gitmoji

@carloscuesta
Copy link
Owner

I would add another column to the layout since we have a lot of space!

@vhoyer
Copy link
Collaborator

vhoyer commented Jul 6, 2020

For point 3 I'm not sure of the design to implement Having the icons a little bit in the center make the raw layout feel strange Or maybe it's not so gross. What do you think?

Sorry, what I meant was that the icons should follow the edge of the current display, so if on grid, should be a little bit centered, and if on list mode, it should be at the end of the list (right side)

@vhoyer
Copy link
Collaborator

vhoyer commented Jul 6, 2020

I would add another column to the layout since we have a lot of space!

It could be, we could have more than just desktop/tablet/mobile breakpoints and have like mobile/tablet/desktop/larger-desktop (:sweat_smile:) so we can fit the third column since I think it would be better try avoiding cutting the text :smile:

@johannchopin
Copy link
Collaborator Author

@vhoyer So I fixed points 1, 3 and 4. For point 2 the main problem of having the buttons following the edge of the current display is that it's not really user-friendly. I you toggle the layout then the buttons are going to move and the user will need to 'follow' the buttons to re-toggle the layout.

@carloscuesta I don't see a layout where we can have 3 columns with all the descriptions. With 2 columns there are already some descriptions that overflow.

@vhoyer
Copy link
Collaborator

vhoyer commented Jul 13, 2020

Still about point 2:

Ok, we should make some benchmarks to see what other players with this kinda behavior did:

Let's see github

Github for one, makes the UI toolbox follow the layout width.

image
image

airbnb

This is kind of a strange example, since it does not have the ability to toggle the layout, but I really just manage to think about github, but if you know one, feel free to comment them :D.

image
image

In Airbnb case, take a look at the header UI elemnts, they follow the content edge.

@vhoyer
Copy link
Collaborator

vhoyer commented Sep 28, 2020

3. Your really like toggle button laughing I don't like the idea of having an additional button for that. As you can see in #453 (comment), a button will look really huge where this feature should be kind of discreet. That's why IMHO it feels better to only have 2 sober icons.

ahah, sorry for the obsession haha, but I'm not talking about the looks, I'm talking about the behavior of a toggle button, in this case we would end up with a toggle icon.

This is mostly to handle focus management.

If you want I can submit a PR to your branch with this suggestion of mine for you to see if you like the behavior :D

@johannchopin
Copy link
Collaborator Author

@vhoyer Yeah sure show me your idea 🚀

@vhoyer
Copy link
Collaborator

vhoyer commented Oct 1, 2020

johannchopin#1 here this is what I meant with "a toggle button" :D

@carloscuesta
Copy link
Owner

I think we can integrate this now! Next to the search input, I can revamp the PR if you don't want to deal with all the conflicts and stuff @johannchopin

@vhoyer
Copy link
Collaborator

vhoyer commented Dec 3, 2020

I think it's a great ideia :D

@johannchopin
Copy link
Collaborator Author

Cool cool I will have a look to the different conflicts asap 👍

@carloscuesta
Copy link
Owner

Feel free, if you need help I can help you 😊

@carloscuesta
Copy link
Owner

Hey @johannchopin I've got some free time today and I I think I can revamp this PR and deliver this feature to the website 😊

Are you ok with that?

@johannchopin
Copy link
Collaborator Author

@carloscuesta hey sorry I had non time this week 😅 so yeah feel free to rewamp it 👍🎉

@carloscuesta
Copy link
Owner

No problem! Cool thanks will send a PR today ❤️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
improvement Website improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Make a cheatsheet style list
4 participants