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

bugfix: positioning responsive icon #637

Merged

Conversation

sharwinbobde
Copy link
Collaborator

@sharwinbobde sharwinbobde commented Dec 17, 2024

bugfix for issue #635.

  • option 1 works because over specialisation for medium size screen.
  • option 2 works because 28 is the correct number 🤓

I suggest keeping both.

Does this count as onboarding?

@sharwinbobde
Copy link
Collaborator Author

sharwinbobde commented Dec 17, 2024

🫦 @thomcsmits

  1. image
  2. image
  3. image

Copy link
Member

@thomcsmits thomcsmits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! :)

Can we add this to outsite.nl as well?

Here for fix1

Here for fix2

@sharwinbobde
Copy link
Collaborator Author

done.... but I saw something I can never unsee
image

@sharwinbobde
Copy link
Collaborator Author

fixed the panda rotation:
image

Copy link
Member

@thomcsmits thomcsmits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Good catch :)

Copy link
Member

@casperboone casperboone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thx for the fixes

<div class="md:w-1/2 px-4">
<PagesHomeIntro />
</div>
<div class="md:w-2/5 overflow-hidden md:overflow-visible relative">
<Panda class="relative md:absolute bottom-0 left-5 w-full" />
<Panda class="relative md:absolute bottom-0 left-5 w-full rotate-[12deg]" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of rotating the panda, I think it's better to fix the positioning for 430px-768px screens (not necessarily with such a specific target). Just to not change the visual appearance of the panda ;) 🐼

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I tried shifting the panda down before rotating it. It looked a bit odd, like just a stub peeking out. I wasn't happy with that, so I said just aligned the image with the diagonal stripe; it was more consistent, looked similar at all resolutions, and we see more of the panda.

Ill try putting up a screenshot with the repositioned panda for comparison later today.

Copy link
Member

@thomcsmits thomcsmits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, can you also add the same tweaks for the mixup site? Not yet incorporated in that PR as I wasn't sure what the rest would review

@casperboone
Copy link
Member

@thomcsmits i don’t think there’s anything to tweak on the mixup page. The surrounding components of the invite aren’t similar, so no relative spacing to set

@thomcsmits
Copy link
Member

What do you mean? It's the same icon position thing that would be fixed with 28px I believe
image

@casperboone
Copy link
Member

casperboone commented Jan 4, 2025

There are no changes to the icon position in this pr. Just the space surrounding the invite component

or I’m missing something

@thomcsmits
Copy link
Member

Didn't check it yet but I presumed the same changes would work if added to these 2 lines:

class="my-8 md:my-4"

<section class="bg-brand-900 text-white text-lg mx-auto pt-12 pb-24 md:flex">

@casperboone
Copy link
Member

casperboone commented Jan 4, 2025

Btw, the icon moving above the text on smaller screens is actually by design 😅 if that’s what you meant @thomcsmits

(and isn’t changed in this pr, just the space surrounding the invite component)

@thomcsmits
Copy link
Member

Ah well that was I think this entire PR😅, based on what I reported here: #635

@casperboone
Copy link
Member

Merging this for now because I want to do a big change to do the codebase. Let's tweak the panda stuff later 😄

@casperboone casperboone merged commit 3be04a7 into dwh-outsite:main Jan 4, 2025
1 check passed
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.

3 participants