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

Issue #730 - Content Sectioning - header #789

Merged
merged 6 commits into from Apr 26, 2018

Conversation

Projects
3 participants
@a2sheppy
Collaborator

a2sheppy commented Apr 23, 2018

Added example for the <header> element.

Issue #730 - Content Sectioning - header
Added example for the `<header>` element.
@welcome

This comment has been minimized.

welcome bot commented Apr 23, 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.

@wbamberg

Thanks @a2sheppy ! The HTML looks good, with a couple of nits. There are a few formatting nits in the CSS, but also the layout breaks at narrower (but not much narrower) widths.

@import url('https://fonts.googleapis.com/css?family=Dancing+Script');
header.pageHeader {
background-color: rgb(200,245,190);

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

4-space indent.

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

Spaces after commas: rgb(200, 245, 190);. Also it would be good to choose one of our standard colors: https://mdn.github.io/mdn-fiori/patterns/scss/variables/.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

I'll fix the space after comma thing. I always use them, so I'm stumped as to where they went. :)

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

LOL -- VS Code is automatically fixing all the 4-space tabs I manually create to be 2-space tabs. I really need to figure out how to make this specific project not do that, I guess, if we're going to insist on sticking with this deviation from standards.

This comment has been minimized.

@schalkneethling

schalkneethling Apr 24, 2018

Collaborator

@a2sheppy I would suggest you add https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode to VS Code. That will solve this problem. You can set the options for Prettier to only format if a .prettierrc file is present, which would then only apply Prettier for this project.

@@ -0,0 +1,32 @@
@import url('https://fonts.googleapis.com/css?family=Dancing+Script');

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

I'm not sure if importing fonts from googleapis might have an effect on performance. Let's ask @schalkneethling :).

This comment has been minimized.

@schalkneethling

schalkneethling Apr 24, 2018

Collaborator

Answered on Discourse. I feel that we should rather not for performance reasons to some degree, but also to keep the number of fonts we use low. It also makes review easier, because the fonts available has already been OK'd

header.pageHeader {
background-color: rgb(200,245,190);
border-bottom: 2px solid rgb(90, 150, 90);

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

Same comment as above re: standard colors.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

I was trying to pick colors to blend somewhat with the photo. I assumed stuff like that would be a reasonable deviation from the standard colors (also not mentioned in the HTML contributing doc, btw; they just mention the mdn-fiori style guide, which also doesn't mention standard colors).

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

The updated PR I will submit later no longer uses these colors but fills the entire header with image.

header.pageHeader {
background-color: rgb(200,245,190);
border-bottom: 2px solid rgb(90, 150, 90);
margin: 0;

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

This seems to make no difference, it's 0 anyway.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

It made a difference for me. Strange. I'll investigate.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

You're right. This one can go. :)

border-bottom: 2px solid rgb(90, 150, 90);
margin: 0;
height: 120px;
}

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

This layout breaks at even quite generous widths:
screen shot 2018-04-23 at 1 54 50 pm

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

Sorry, didn't know we were supposed to have perfected layout given the space constraint. Will adjust it.

main > article > p:first-of-type {
margin-top: 0;
}

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

File should end with a newline.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

Did not find a mention of that in the contributing doc. It should be added then.

@@ -0,0 +1,13 @@
<header class="pageHeader">
<img src="/media/examples/puppy-header-logo.jpg">

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

Can you please confirm that we are allowed to use this image?

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

We are; however, I forgot to include attribution. I will add it as a comment in the CSS.

</header>
<p>Like really, a lot. They're adorable and their ears are so, so snuggly soft!</p>
</article>
</main>

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

File should end with a newline.

"exampleCode": "live-examples/html-examples/content-sectioning/header.html",
"cssExampleSrc": "live-examples/html-examples/content-sectioning/css/header.css",
"fileName": "header.html",
"title": "HTML Demo: &lt;header&gt;",

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

Since #777 landed, we don't need to encode < and > any more.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

Contributor doc needs to be updated then.

This comment has been minimized.

@wbamberg

wbamberg Apr 26, 2018

Member

Please un-encode these characters.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 26, 2018

Collaborator

Whoops - thought I saw on IRC that they still needed to be encoded pending a production push, so I left it alone. Fixing it now.

}
header.pageHeader > h1 {
font: bold 42px "Dancing Script", cursive;

This comment has been minimized.

@wbamberg

wbamberg Apr 23, 2018

Member

Wee seem to discourage people from using pixels for font-size: https://developer.mozilla.org/en-US/docs/Web/CSS/font-size#Pixels

This comment has been minimized.

@a2sheppy

a2sheppy Apr 23, 2018

Collaborator

Okay, this has been informative. I hadn't realized just how much we were trying to make all the things ideal here, rather than focusing on just making the example show the element in question. I will work to make these more adherent to all the things going forward.

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

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

Address review concerns
This commit corrects the following issues:

* Indentation fixed
* Removed use of background color and the border line.
  instead, the header image is used to fill the entire
  header background
* The Dancing Script font is now local
* Substantial rework of the CSS to optimize as much as
  possible
* Fixed problems with the layout breaking at narrow
  widths
* Shortened up the HTML by getting rid of the fake
  blog post and just having short body text instead
* Added attribution for the background image
background: no-repeat left/cover url(/media/examples/puppy-header-logo.jpg);
height: 120px;
width: 100%;
min-width: 380px;

This comment has been minimized.

@wbamberg

wbamberg Apr 25, 2018

Member

Formatting nit: this looks like 2 spaces and a tab, not 4 spaces.

header.page-header > h1 {
font: bold 3em "Dancing Script", cursive, fantasy;
position: absolute;
width: 100%;

This comment has been minimized.

@wbamberg

wbamberg Apr 25, 2018

Member

Formatting nit: this looks like 2 spaces and a tab, not 4 spaces.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 25, 2018

Collaborator

Weird. I'll fix these. Not sure how it happened.

width: 100%;
text-align: center;
margin: 0;
bottom: calc(60px - 0.7em);

This comment has been minimized.

@wbamberg

wbamberg Apr 25, 2018

Member

Formatting nit: this looks like 2 spaces and a tab, not 4 spaces.

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 25, 2018

Thanks for the updates @a2sheppy , this looks great. But there are still some issues with the layout I think.

https://developer.mozilla.org/en-US/docs/User:wbamberg/HTML_editor_user_test_pages/datalist shows what the editor will look like in an MDN page. In that page, with a window width of 1440 px, the entire editor gets about 1000px of width, and the output pane gets about 375px.

At that width, the header is overflowing and getting cut off:

screen shot 2018-04-25 at 1 50 26 pm

I think we should aim to have the examples work well at this width.

Of course it's possible that people will have a narrower browser window, and ideally it should work with that, too. The CSS examples switch mode from side-by-side to top-and-bottom with the editor at about 730px, which would give the HTML output window only about 260px. Having the layout work at that width is potentially quite hard to achieve, and if it's not practical then I think it's OK, but we should consider it.

I wonder if we could use flexbox to make the layout more responsive, like:

<header class="page-header">
    <img src='/media/examples/puppy-header-logo.jpg'>
    <h1>Cute Puppies Express!</h1>
</header>

<main>
    <p>I love Beagles <strong>so</strong> much! Like really, a lot.
        They're adorable and their ears are so, so snuggly soft!</p>
    <p>Photo licensed by Wikimedia Commons under the
        <a href="https://creativecommons.org/licenses/by-sa/3.0/deed.en">
        CC-ShareAlike 3.0 Unported license</a></p>
</main>

@font-face {
    font-family: "Dancing Script";
    src: url('/media/examples/DancingScript-Regular.ttf');
}

header.page-header {
    display: flex;
    height: 120px;
    align-items: center;
    color: #fff;
    text-shadow: #000 0 0 0.2em;
    background-color: green;/*or whatever*/
}

header.page-header > h1 {
    font: bold 2em "Dancing Script", cursive, fantasy;
    margin: 2%;
}

header.page-header > img {
    flex: none;
    width: 10em;
}

main {
    font: 1em "Lucida Grande", "Arial", "Helvetica", sans-serif;
}

What do you think? (this also reduces the h1 font size because 3em will never fit)

@a2sheppy

This comment has been minimized.

Collaborator

a2sheppy commented Apr 25, 2018

Huh. I tested this thing like crazy and never had it get cut off at all. That's disappointing. Anyway, I was trying to avoid flexbox but agree it's probably the way to go here. I actually don't know why I was trying to avoid it, since it's sort of the "right" way to do this stuff. :)

I'll update and submit a revision shortly.

Use a flexbox layout and calc() with vw units to set
the size of the header's font so that the text is
scaled between 1em and 3em depending on the window
width. Use a short, wide image as the backdrop. Change
the body to match other examples.

Moved font-face stuff to the end so it's not hogging
valuable space above the fold.
background: no-repeat left/cover url(/media/examples/puppy-header-logo.jpg);
display: flex;
height: 120px;
min-width: 120px;

This comment has been minimized.

@wbamberg

wbamberg Apr 26, 2018

Member

I think you don't need this line (it's also got a tab in)

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 26, 2018

This works great! r+ with a couple of nits, noted above.

Replaced a stray tab with two spaces, and switched to
using single quotes. Also outright removed quotes in
a couple of places since they were optional there
anyway.
background: no-repeat left/cover url(/media/examples/puppy-header-logo.jpg);
display: flex;
height: 120px;
min-width: 120px;

This comment has been minimized.

@wbamberg

wbamberg Apr 26, 2018

Member

I don't think we need this line at all.

This comment has been minimized.

@a2sheppy

a2sheppy Apr 26, 2018

Collaborator

We do. Without it, the text escapes the header box at very narrow widths.

This comment has been minimized.

@wbamberg

wbamberg Apr 26, 2018

Member

What very narrow width? At about 180px, it looks the same with or without this line, and I don't think it is expected to work at narrower widths than this (at this point you are getting 1-2 words per line, so it isn't very usable anyway).

@wbamberg

Thanks @a2sheppy , this looks great!

@wbamberg wbamberg merged commit cc82cbc 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

@a2sheppy a2sheppy deleted the a2sheppy:html-header-element branch Apr 27, 2018

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