-
Notifications
You must be signed in to change notification settings - Fork 509
CSS interactive examples: Add example for width. #571
Conversation
|
💖 Thanks for opening this pull request! 💖 |
wbamberg
left a comment
There was a problem hiding this 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; | |||
There was a problem hiding this comment.
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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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.
|
Those changes seem fine :) I'll add them fairly soon. |
wbamberg
left a comment
There was a problem hiding this 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!
|
Congrats on merging your first pull request! 🎉🎉🎉 |
* 'master' of https://github.com/mdn/interactive-examples: CSS interactive examples: Add example for width. (mdn#571)
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.