Skip to content
This repository was archived by the owner on Oct 17, 2025. It is now read-only.

Conversation

@rachelandrew
Copy link
Collaborator

An initial example for the set of examples I'm building.

@welcome
Copy link

welcome bot commented Feb 1, 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.

Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

Thanks @rachelandrew - Two small nits, other than that, this looks great!

border: 3px solid blue;
width: 100px;
margin: 10px;
} No newline at end of file

Choose a reason for hiding this comment

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

Nit: missing empty line at the end of the file

"type": "css"
}
}
} No newline at end of file

Choose a reason for hiding this comment

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

Nit: missing empty line at the end of the file

@wbamberg
Copy link
Contributor

wbamberg commented Feb 2, 2018

This looks great, the only thing I'd say is to make sure it looks the way we want when the output window is at its minimum width.

On MDN pages the editor is laid out "side by side": that is with the example choices on the left and the output on the right, as it is in the local server that's started by npm run start. Then if the page width goes below some threshold it switches to "top and bottom", with the example choices above and the output below.

I'm not sure what the exact threshold is, but it seems to give us a minimum output window width of about 285 pixels. At that width the flex items wrap into a single column, which cuts off the top and bottom of the container:

screen shot 2018-02-01 at 3 29 11 pm

I wasn't sure if this was intentional, but if not perhaps it would be good to make the items narrower?

@rachelandrew
Copy link
Collaborator Author

That's probably doable for this one but I can think of examples (especially in grid layout) that will be difficult to make so narrow and still useful.

Also, did I miss documentation about minimum widths? If that doesn't exist perhaps it should be added somewhere.

@schalkneethling
Copy link

Also, did I miss documentation about minimum widths? If that doesn't exist perhaps it should be added somewhere.

Very good point @rachelandrew I was thinking the exact same thing as I was reading the comments. I believe this has not been at the top of our minds as we were focus on the JS examples. I will add an issue to mention this clearly in the docs.

@wbamberg
Copy link
Contributor

wbamberg commented Feb 2, 2018

That's probably doable for this one but I can think of examples (especially in grid layout) that will be difficult to make so narrow and still useful.

I think that's fine, we should be pragmatic about it :).

Also, did I miss documentation about minimum widths? If that doesn't exist perhaps it should be added somewhere.

No, I don't think it does, and yes, we should add it to the contributing guide.

@rachelandrew
Copy link
Collaborator Author

ok - I've updated this for both of those issues.

Copy link

@schalkneethling schalkneethling left a comment

Choose a reason for hiding this comment

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

r+ Thanks @rachelandrew

@schalkneethling schalkneethling merged commit efcb505 into mdn:master Feb 2, 2018
@welcome
Copy link

welcome bot commented Feb 2, 2018

Congrats on merging your first pull request! 🎉🎉🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants