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

feat: Refactored gallery to be pure CSS. Fixes JS bug where code would gallery would only resize on scroll #56

Conversation

TeaDrinkingProgrammer
Copy link
Contributor

@TeaDrinkingProgrammer TeaDrinkingProgrammer commented Apr 21, 2024

Started as a fix for #54, ended up as a complete CSS rewrite for the gallery component.
Some considerations:

  1. I have tested the layout on both large screens and mobile size, but it is probably a good idea to confirm that this works on other websites.
  2. I think the functionality is similar enough to the JS implementation, but if you want an exact copy, you might want to tweak it more.
  3. The layout is made possible by Flexbox and object-fit. This makes it incompatible with IE. For my personal use case, this is an easy sacrifice, but I leave it up to you to decide this for this project.

Based on: https://css-tricks.com/adaptive-photo-layout-with-flexbox/

@tfsomrat
Copy link
Contributor

Nah bro, it's completely break the gallery

CleanShot 2024-04-22 at 09 22 53

@TeaDrinkingProgrammer
Copy link
Contributor Author

What do you mean break? It is not the same as the last version but it is a working gallery. That's what I tried to explain in the merge request.

@tfsomrat
Copy link
Contributor

I am sorry, but it's not looking good. I reverted the PR. thanks for your contribution anyway. have a good day.

@TeaDrinkingProgrammer
Copy link
Contributor Author

Can you explain what you would like to change? The current JS version isn't working correctly so if we can change this to something that works for you it would improve the developer experience.

@tfsomrat
Copy link
Contributor

We can't change the look of the gallery or slider. and all the functionality that it's proving right now.

@TeaDrinkingProgrammer
Copy link
Contributor Author

I'll see if I can make it more similar to the current version. Can you open the PR again?

@tfsomrat
Copy link
Contributor

I didn't close the PR, I was merged it. so it can't be re-open.

@TeaDrinkingProgrammer
Copy link
Contributor Author

Ah yes, that's right.

@TeaDrinkingProgrammer
Copy link
Contributor Author

Could you point me to the example you showed? Then I can test it.

@tfsomrat
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants