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

Conversation

@dominicduffin1
Copy link
Contributor

I've created the interactive example based on the four blue boxes as per #565

This is my first ever pull request so I hope everything's alright. I'm pleased to take any feedback.

@welcome
Copy link

welcome bot commented Feb 10, 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
Contributor

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Looks good @dominicduffin1 , thanks for your contribution! Just a few minor comments :).

@@ -0,0 +1,6 @@
#example-element {
background-color: #E4F0F5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: use lowercase for the letters: #e4f0f5.

@schalkneethling , is there a style guide we can link from the contributing docs? It seems unfair to complain about something we don't document.

height: 80%;
justify-content: center;
color: #000000;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to vertically-center the text. You could borrow what the border-radius example does, by adding:

 display: flex;
 justify-content: center;
 flex-direction: column;

You might like to use the same color scheme that border-radius uses, too. But that's entirely up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the colour scheme on border-radius is better.


<div id="output" class="output large hidden">
<section id="default-example" class="default-example">
<div id="example-element" class="transition-all">This is a light blue box</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Other similar examples have text like "This is a box with a border around it.". For consistency, you could change this text to "This is a box whose width you can change.", or something like that.

@dominicduffin1
Copy link
Contributor Author

Those changes seem fine :) I'll add them fairly soon.

Copy link
Contributor

@wbamberg wbamberg 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 to me @dominicduffin1 ! Thanks for your contribution to MDN!

@wbamberg wbamberg merged commit d963036 into mdn:master Feb 11, 2018
@welcome
Copy link

welcome bot commented Feb 11, 2018

Congrats on merging your first pull request! 🎉🎉🎉

dominicduffin1 added a commit to dominicduffin1/interactive-examples that referenced this pull request Feb 12, 2018
@dominicduffin1 dominicduffin1 deleted the add-width-css-examples branch February 12, 2018 10:08
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.

2 participants