unixporn-dots / unixporn-dots.github.io

A collection of dotfiles and icons from the r/unixporn community
https://unixporn-dots.github.io
111 stars 19 forks source link

feat: Move buttons in card header #33

Closed ManorSailor closed 2 years ago

ManorSailor commented 2 years ago

This commit closes issue #31. The following changes have been made:

In issue #31 , @Pesc0 mentioned about replacing the image preview button with clickable image for preview. I haven't got around doing that yet, although, I will work on it after @Pesc0 JS refactor have been merged.

All of the changes made have been tested on latest firefox & (almost) latest version of Ungoogled Chromium. Everything is working fine, if something does break, let me know. I will take look :)

ArmoredVortex commented 2 years ago

32 has been merged now. There's only 1 conflict arising cause some of the stuff from index.js has been moved to cardGenerator.js.

ManorSailor commented 2 years ago

32 has been merged now. There's only 1 conflict arising cause some of the stuff from index.js has been moved to cardGenerator.js.

I will fix it now then

Rahuletto commented 2 years ago

Can we see the resultant image/screenshot ?

ManorSailor commented 2 years ago

Can we see the resultant image/screenshot ? Screenshot (5)

Rahuletto commented 2 years ago

Doesn't it give problems to smaller display ? (There are many Dotfiles that has bigg title names)

And yea seems like there is no tag at the hovering card (maybe u missed it idk)

ArmoredVortex commented 2 years ago

And yea seems like there is no tag at the hovering card (maybe u missed it idk)

It's actually intentional so it doesn't hinder the complete view

ManorSailor commented 2 years ago

Doesn't it give problems to smaller display ?

Good catch. I nearly forgot about mobile devices. I will turn this into a draft PR for the time being

And yea seems like there is no tag at the hovering card (maybe u missed it idk)

I have set it opacity to 0 on hover. Thought, it look better when the user is hovering over the image

Edit: As for how it will look on mobile devices, we can make the icon a bit smaller & show them by default since mobile devices cannot have hover

Rahuletto commented 2 years ago

Or what I have in my mind is maybe make the buttons vertical at left. But still description may hit the buttons

ArmoredVortex commented 2 years ago

image This reminds me of #9 when we had tags on the header itself

ManorSailor commented 2 years ago

@Rahuletto Alright. I'll try both approaches & then we can decide which one works better. Btw, I just noticed that on mobile devices, you've to tap on cards to make their description & buttons visible. Is it intentional?

Cause, I was thinking that maybe for mobile devices we can make bigger cards & show the information by default?

What do you think?

Rahuletto commented 2 years ago

@Rahuletto Alright. I'll try both approaches & then we can decide which one works better. Btw, I just noticed that on mobile devices, you've to tap on cards to make their description & buttons visible. Is it intentional?

It is not intended but it looks cool. Touch is exactly hover in mobile browsers.

ArmoredVortex commented 2 years ago

Well i came across this quick but dirty fix. Removing the image button creates extra space for the title to fill in and eliminates the weird hover behaviour since we plan on removing the button eventually anyway

Rahuletto commented 2 years ago

Cause, I was thinking that maybe for mobile devices we can make bigger cards & show the information by default?

But most phones nowadays has less width and more height. So making big cards at same ratio is a big challenge. And showing the information by default may make things clutter

Rahuletto commented 2 years ago

Well i came across this quick but dirty fix. Removing the image button creates extra space for the title to fill in and eliminates the weird hover behaviour since we plan on removing the button eventually anyway

Removing all buttons gives more room 😳

ManorSailor commented 2 years ago

@ArmoredVortex Huh, I see. Do not merge this PR yet. I've a few other things in mind which I can try & see how they look. I'll keep you guys updated.

Also, the preview image button can be removed, I'll add image preview to the card image itself.

Pesc0 commented 2 years ago

Looks good! I really like having icons instead of text, though the description seems to get in the way still. Problem is we cant remove everything, something to interact with has to remain.

How about on hover:

also

.card:hover {
transform: scale(1.05)
}

might look nice

Just throwing out a couple of ideas there

Rahuletto commented 2 years ago
.card:hover {
transform: scale(1.05)
}

might look nice

Just throwing out a couple of ideas there

Isn't it gonna ruin the grid ? Making other cards expand or have weird size shifts ?

Pesc0 commented 2 years ago

Seems to work fine on latest firefox. This is needed as well:

.card {
  ...
  transition: 0.2s ease-in-out;
}

Can you confirm it works?

ManorSailor commented 2 years ago

@ArmoredVortex @Rahuletto Have a look at this. I have removed the image icon from the cards & added image preview ability directly to the image itself. Although, there is no visual feedback apart from the cursor turning into a pointer. Also, I have been able to stop the words from breaking down unto a new line. Screenshot (6) Screenshot (9)

ArmoredVortex commented 2 years ago

Good Job, I did try it and it only breaks for titles with above 28 chars (at minimum card length). Which i think is a fairly reasonable limit.

I did however come across this bug image Doesn't seem to be the issue with the main branch plus it only happens when there's not enough cards for the grid (for example while searching)

ArmoredVortex commented 2 years ago

The above issue is fixed now, Let me know if you think this is ready to be merged.

ManorSailor commented 2 years ago

@ArmoredVortex Removing min height from themes container will break the smooth scrolling fix. Due to the way cards are generated, it makes the browser scroll to top abruptly.

To fix this abrupt behavior now, a different workaround will be required. Although, I think, that can be done at a later time.

ArmoredVortex commented 2 years ago

Woops i thought setting the max-height for the card instead would work but i encountered some broken description behavior. I'll just remove the min-height for the container for now until a fix is available.

Rahuletto commented 2 years ago

Uhm.. now u can't really see the description (or run the hover event) of the cards in mobile because there is no room to hover (touch) as the image just shows the image preview

ManorSailor commented 2 years ago

@Rahuletto @_@ Right. Any ideas? We might have to readd the button for image preview back for mobile devices