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

Fixes issue 38 - Added Contributors Showcase Component #39

Merged
merged 6 commits into from Dec 3, 2019

Conversation

drwm-base
Copy link
Contributor

Fixes Issue #38

I made a component and added it to the Get Involved section of the main page for now. I know you are working on a Get Involved page, so making this a component makes it easy to work with. I had to add gatsby-transform-json to the plugins in order to read the json data, and I created a contributors.json file in src/data in order to store the json data.

@flowirtz
Copy link
Contributor

@drwm-base Amazing!! Thanks so much!
I'll give you a review this evening once I'm home, was gone for the weekend.

Copy link
Contributor

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

Very cool, thank you @drwm-base!
I left a few comments, mostly code style related and/or nits.

It still looks a little weird when looking at it on my mobile, see below screenshot.
The second image in each row seems to scale fine, the others don't really.
Tbh no idea how the grid magic ✨ works, but could you look into it again?

Maybe we could even achieve something where it shows you 7 images next to each other on desktop, 5 on medium sized browsers and something like 2 or 3 on mobile next to each other. But that's just CSS blackmagic and I have no idea how that works - if you feel like looking into this: AMAZING! If not, no worries. But let's at least fix the weird scaling on mobile. 😊

Edit. Maybe the (tailwind) flexbox stuff is helpful for this? 🤔

image

Thanks again for taking this big one on, @drwm-base!

@drwm-base
Copy link
Contributor Author

I will address everything here and get back to you asap!

@flowirtz
Copy link
Contributor

Sounds great, no rush please. This isn't urgent.

@drwm-base
Copy link
Contributor Author

Hey so i fixed some of the issues you brought up, I couldn't get tailwind to work properly, I'm gonna continue to look at it this week. For now, I have a css solution using @media that does what you want it to do without stretching.

@drwm-base
Copy link
Contributor Author

drwm-base commented Nov 28, 2019

@fwirtz hey, so I looked into it and I can't really find a solution using tailwind. The core functionality of the grid you wanted is there, and all problems have been resolved. Tailwind is not working because of an issue with compatibility with the .module.css style format used by React. Essentially tailwind works with class names, but the only class name that can be used must be grabbed from the style module.

And the weird scaling on mobile should be fixed.

@drwm-base drwm-base mentioned this pull request Nov 28, 2019
4 tasks
Copy link
Contributor

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

Looks very good @drwm-base, thank you!
Just some teeny tiny nits left.

I'm still not sure whether tailwind is the best way forward, so far it rather scared people off than incentivise them to collaborate. I'm also new to it. Maybe we should look into a better CSS Strategy.
Very open to suggestions.

@drwm-base
Copy link
Contributor Author

@fwirtz I have applied your changes.

@flowirtz flowirtz merged commit 02416b1 into openclimatefix:master Dec 3, 2019
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.

None yet

2 participants