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

The footer #60

Merged
merged 18 commits into from Apr 3, 2020
Merged

The footer #60

merged 18 commits into from Apr 3, 2020

Conversation

michalczaplinski
Copy link
Member

@michalczaplinski michalczaplinski commented Mar 23, 2020

The "footer" component of the website. It gets the content from template-parts gutenberg block.

@michalczaplinski michalczaplinski self-assigned this Mar 23, 2020
@michalczaplinski michalczaplinski linked an issue Mar 23, 2020 that may be closed by this pull request
@frontibotito
Copy link
Member

frontibotito commented Mar 23, 2020

Deploy preview for website ready!

Built with commit fbeefc5

https://frontity-org-nxh9a5xqv.now.sh

Copy link
Member

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

Looks nice so far. What I think it's left:

  • Add the correct padding to the footer. Although we are going to remove the rule applying to every inner div, the footer will still need its own padding.
  • We may need to set a max-width: 1080px to the horizontal-separator.
  • Align correctly the last row of the footer.
  • Change the color on hover of the links.

@michalczaplinski michalczaplinski changed the title WIP: The footer The footer Mar 26, 2020
@michalczaplinski
Copy link
Member Author

Looks nice so far. What I think it's left:

  • Add the correct padding to the footer. Although we are going to remove the rule applying to every inner div, the footer will still need its own padding.

OK, I will add this rule but I don't think it will apply at the moment, because the global rule has a higher priority! Will you update the global rule or should I have a look?

  • We may need to set a max-width: 1080px to the horizontal-separator.

👍

  • Align correctly the last row of the footer.

I'm sorry, I'm not sure what you mean there 🙂 Align it to what? Align horizontally or vertically?

  • Change the color on hover of the links.

According to the design, the links should have a blue color on hover like I added. Did I miss something?

@SantosGuillamot
Copy link
Member

SantosGuillamot commented Mar 27, 2020

Will you update the global rule or should I have a look?

I will do it, don't worry 🙂

I'm sorry, I'm not sure what you mean there slightly_smiling_face Align it to what? Align horizontally or vertically?

According to the design, the links should have a blue color on hover like I added. Did I miss something?

I think the now domain wasn't updated, I couldn't see these changes yesterday. Sorry for that. It looks nice now 👍

It seems there are some issues with the tests.

@luisherranz luisherranz requested review from DAreRodz and removed request for DAreRodz March 27, 2020 16:11
@michalczaplinski
Copy link
Member Author

@SantosGuillamot @iamuchejude Please review 🙂

Copy link
Member

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

The footer padding isn't matching the design (I've removed locally the global styles that were overriding it). Apart from that, we have to take a look the last row element, some padding and the two images in the mobile version.

We can take a look at it on Monday, and see together how to check the paddings in Invision.

@SantosGuillamot
Copy link
Member

I've created a Pull Request to solve the problem with css that is causing troubles (#65). You can merge it to check how it'd be. I can see that we may need to add the max-width property directly to the footer, as the css that was adding it before it's not applying anymore.

@michalczaplinski
Copy link
Member Author

The footer padding isn't matching the design (I've removed locally the global styles that were overriding it). Apart from that, we have to take a look the last row element, some padding and the two images in the mobile version.

I think that the version in the design is simply wrong, because I see no reason why the "Accelerated by Google. Invested by Automattic." should be missing in the mobile version of the footer 🙂

@michalczaplinski
Copy link
Member Author

I also think that the designer did not leave enough padding between the images in the mobile version, but I think it's such a minor detail that I would not worry about it honestly 🙂

@SantosGuillamot
Copy link
Member

Yes, I wouldn't worry that much about that neither, we can easily change that. We have to adapt the max-width and padding of the desktop version as well.

@michalczaplinski
Copy link
Member Author

@SantosGuillamot I've updated the styling a little more and I think it should be just fine now 👌

We still have to adjust the global padding on the homepage, but that's work for later 🙂

Copy link
Member

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

Looks great Michal! 🙂

@michalczaplinski
Copy link
Member Author

@iamuchejude Could you give me a review? 🙂

Copy link
Contributor

@iamuchejude iamuchejude left a comment

Choose a reason for hiding this comment

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

Good work @michalczaplinski

I noticed just a few things though. The footer links are not aligned with the logo when the screen width is in between 600px and 799px.

Footer margin

Also, the footer links are also rendered as <p> element. I think that's not really cool for accessibility. For example, if you use a screen recorded for the website, the links will be read as paragraphs and other reason too.

@michalczaplinski
Copy link
Member Author

michalczaplinski commented Apr 1, 2020

Oh, good points about the HTML elements and the fr widths @iamuchejude, thank you!

I've changed the elements to ul li.

The footer links are not aligned with the logo when the screen width is in between 600px and 799px.
About this, it's because the default gutenberg CSS adds the following rule:

@media (min-width: 600px) {
  .wp-block-column:nth-child(2n) {
     margin-left: 32px;
  }
}

I'm gonna keep it as is, so that in wider resolutions we get the padding as intended. In the end, "it's just a footer" so I think this is a super minor detail that has very little impact 🙂

@SantosGuillamot
Copy link
Member

Just FYI: We're finally going to add the newsletter as a reusable block, and include it in the footer. I've included it in the Footer HTML, but it will be handled by another issue #70 .

@michalczaplinski
Copy link
Member Author

@iamuchejude Could you review it? 🙂

Copy link
Contributor

@iamuchejude iamuchejude left a comment

Choose a reason for hiding this comment

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

Good work @michalczaplinski

@michalczaplinski michalczaplinski merged commit 7657725 into dev Apr 3, 2020
@michalczaplinski michalczaplinski deleted the footer branch April 3, 2020 18:34
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.

Footer
4 participants