search

wagtail / areweheadlessyet

Are we headless yet?
https://areweheadlessyet.wagtail.org
MIT License
19 stars 4 forks source link

Initial HomePage styling #1

Closed Tijani-Dia closed 2 years ago

Tijani-Dia commented 2 years ago

Style all hardcoded components in the HomePage

thibaudcolas commented 2 years ago

@Tijani-Dia additional review comments based on testing the page in Chrome & Safari:

Tijani-Dia commented 2 years ago

Thanks @thibaudcolas and @jhancock532 for your thorough reviews! I've tried to address most of your comments.

@thibaudcolas

The tag is missing a lang="en"

I addressed this following what has been described here.

The styles of the "highlight" divider’s linecaps don’t look correct to me

What do you mean by highlight divider’s linecaps?

The hear emoji appears squashed for me. I think we should probably switch back from the PNG to the text symbol instead, since we’re doing this for the thumbs up.

Addressed.

On smaller viewport widths, the logo appears underneath the headline text.

Can you share a screenshot of this please?

Please take a moment in your PR description to describe the changes – which tickets your work corresponds to, anything that’s intended to still be a work-in-progress vs. finished, how you’ve tested this, how you expect reviewers to test the changes

I didn't want to link to the corresponding ticket since people who don't have access to the project (in Codebase) won't be able to access it. I agree that I could describe better the changes. To test it, the main criteria is that the design matches the one in Figma. This ticket only includes hardcoded components.

I will open an issue to add metadata as suggested by @jhancock532.

The main point remaining from your code review is to make SVG inline and I'm currently looking atthe best way to do that (like making a new component for SVGs...)

thibaudcolas commented 2 years ago

@Tijani-Dia sorry, I should have shared screenshots right away 🤦 here we are.

I believe this should be round on the ends:

Screenshot 2022-02-03 at 16 09 30

And here is the logo overlapping:

Screenshot 2022-02-03 at 16 09 20

Tijani-Dia commented 2 years ago

@thibaudcolas I've addressed the styling issues mentioned in the previous comment.

Tijani-Dia commented 2 years ago

Thanks @thibaudcolas. I've done the logo with inline SVG in the following PR.

thibaudcolas commented 2 years ago

Ah nice :)