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

Fix country flag placement in meetup list [Fixes #11861] #11862

Merged
merged 6 commits into from
Jan 18, 2024

Conversation

hursittarcan
Copy link
Contributor

@hursittarcan hursittarcan commented Jan 3, 2024

Description

  • Fixes the strange placement of the country flags -> described in issue Fix country flag placement in Ethereum meetups list #11861.

  • I fixed this by removing the line-height from the emoji component (used to display the country flags). I wasn't sure about removing the line-height entirely because the component is being used on many different places too, soo I added an extra prop to check for flags.

Related Issue

add extra prop to check if emoji is a flag - fixes strange placement on meetup page
specify emoji is a country flag
Copy link

netlify bot commented Jan 3, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit d9ae553
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/65a847903161e40008816ce0
😎 Deploy Preview https://deploy-preview-11862--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nloureiro
Copy link
Contributor

@corwintines this fix looks good, it fixes the issue in questions and make the list look better.

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Hey @hursittarcan! Thanks for dropping this fix... This does fix the issue, but I would push back on altering the Emoji component here for this one-off use case.

The Emoji component spreads {...props} into the component, so instead of making a whole new boolean prop here, we can just pass lineHeight when declaring an Emoji to override this prop specifically.

I would revert everything changed in Emoji.tsx, and instead just change isFlag to lineHeight="unset" inside it's usage within MeetupList. lineHeight will be passed to the Emoji component accomplishing the same thing you're doing here, but without altering the component itself.

src/components/MeetupList.tsx Outdated Show resolved Hide resolved
hursittarcan and others added 2 commits January 17, 2024 22:31
Co-authored-by: Paul Wackerow <54227730+wackerow@users.noreply.github.com>
@hursittarcan
Copy link
Contributor Author

Hey @wackerow 👋
Thanks for the review!
Did the requested changes 👍
And wow, your solution was way cleaner than mine, didn't realise it could be this simple. 😅

Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

And wow, your solution was way cleaner than mine, didn't realise it could be this simple. 😅

Hah, glad I could help =) Thanks again!

@corwintines corwintines merged commit f810173 into ethereum:dev Jan 18, 2024
9 of 10 checks passed
Copy link

gitpoap-bot bot commented Jan 18, 2024

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

Be sure to join the Ethereum.org discord if you are interested in contributing further to the project or have any questions for the team.

GitPOAP: 2024 Ethereum.org Contributor:

GitPOAP: 2024 Ethereum.org Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

This was referenced Jan 31, 2024
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.

4 participants