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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add <footer> example #804

Merged
merged 6 commits into from Apr 26, 2018

Conversation

Projects
3 participants
@2alin
Contributor

2alin commented Apr 24, 2018

Hi, this is my first contribution to this repo. Hope I'm doing it right.
Fixes #728
edit: I just saw that @a2sheppy assigned it himself a day ago 馃槄, sorry. Feel free to reject the request if that's a problem.

@welcome

This comment has been minimized.

welcome bot commented Apr 24, 2018

馃挅 Thanks for opening this pull request! 馃挅
Here is a list of things that will help get it across the finish line: - If this is a new or updated CSS interactive example, please ensure that you followed the CSS styleguide - If this is a new or updated JavaScript interactive example, please ensure that you followed the JavaScript styleguide - If your changes affects any of the steps in our contribution docs, please also make the relevant changes there.

@schalkneethling schalkneethling added this to To do in Q2 - Sprint 2 via automation Apr 25, 2018

@schalkneethling schalkneethling added this to the Quarter 2 ~ Sprint 2 milestone Apr 25, 2018

@schalkneethling schalkneethling self-requested a review Apr 25, 2018

@schalkneethling schalkneethling self-assigned this Apr 25, 2018

@schalkneethling

@wbamberg I really like this example but, it does use a lot of HTML and CSS. Is it to much?

h1 {
font-size: 1.8em;
margin-bottom: 0.5em;

This comment has been minimized.

@schalkneethling

schalkneethling Apr 25, 2018

Collaborator

Nit: Code style: Zero based values should omit the leading 0 ~ 0.5em => .5em

display: flex;
padding: 1em .5em;
margin: 1.5em 0 0;
font-size: 0.9em;

This comment has been minimized.

@schalkneethling

schalkneethling Apr 25, 2018

Collaborator

Nit: Code style: Zero based values should omit the leading 0 ~ 0.5em => .5em

Also, prefer rem based units for font sizes.

2alin added some commits Apr 25, 2018

@2alin

This comment has been minimized.

Contributor

2alin commented Apr 25, 2018

Done, changed to rem units in font size, px in padding/margin and no more leading 0. Also changed back the line in package.json

@2alin

This comment has been minimized.

Contributor

2alin commented Apr 25, 2018

@schalkneethling and if you think html/css is too much. I could remove the anchor tags in the footer, so the elements there end up as list items only (no clickable). That will reduce horizontal space in the html, and css will go from 40 lines to 30 lines of code.
What do you think?

edit: and I could remove 3 more lines of css: 1 line for font family that's redundant (sans-serif), 2 for margins that won't affect the overall visual aspect of the demo. That will make the css 27 lines in total.

<ul>
<li><a href="http://example.com/terms">Terms</a></li>
<li><a href="http://example.com/blog">Blog</a></li>
<li><a href="http://example.com/about">About</a></li>

This comment has been minimized.

@schalkneethling

schalkneethling Apr 25, 2018

Collaborator

@2alin Yes, let's make these just list items. Then you can also remove lines 32-40 of the CSS file. Thanks so much.

2alin added some commits Apr 25, 2018

@2alin

This comment has been minimized.

Contributor

2alin commented Apr 25, 2018

done, now html/css are both cleaner 馃槈

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 25, 2018

It's a great example :).

I really like this example but, it does use a lot of HTML and CSS. Is it to much?

As it is I think this is fine.

@schalkneethling

Love this. Great work @2alin r+

@schalkneethling schalkneethling merged commit 407c810 into mdn:master Apr 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Q2 - Sprint 2 automation moved this from To do to Done Apr 26, 2018

@welcome

This comment has been minimized.

welcome bot commented Apr 26, 2018

Congrats on merging your first pull request! 馃帀馃帀馃帀

@2alin

This comment has been minimized.

Contributor

2alin commented Apr 26, 2018

Thank you (both) for the review 馃榾

@2alin 2alin referenced this pull request Apr 29, 2018

Merged

Update <footer> example CSS #845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment