Adding list-style css example. #547

helmutgranda · 2018-02-06T17:35:31Z

Adding list-style css example to support ticket issue #484.

schalkneethling

One suggestion but, other than that, this looks great. Thanks @helmutgranda

schalkneethling · 2018-02-07T11:42:29Z

live-examples/css-examples/lists-and-counters/list-style.html

+
+<div id="output" class="output large hidden">
+    <section id="default-example" class="default-example">
+            <ul id="example-element" class="transition-all">


I would suggest adding some custom styling for the #example-element here as described in https://github.com/mdn/interactive-examples/blob/master/CONTRIBUTING.md#styling-the-example

All that is need is a fuller width, and bumping the font-size so that the image in the last example is clearer:

.default-example { font-size: 1.8rem; } #example-element { width: 50%; }

@schalkneethling, updates applied. I used 80% for example-element's width instead of 50% to prevent wrapping specially with little-roman example.

wbamberg · 2018-02-07T15:15:12Z

@schalkneethling , I had some comments on this as well, so please don't merge until I have had a chance to write them down :).

wbamberg · 2018-02-07T19:02:36Z

Thanks again for this contribution! I think this is also a tricky one, as it has quite complex syntax and it's challenging to choose just 6 representative values.

I had a few comments. I'll use a separate comment for each one, which I hope will make them more usable.

wbamberg · 2018-02-07T19:03:04Z

A couple of syntax variants that we could perhaps include, that are not in the current list:

just position
all three: position, image, type
another position, image, type, but where the image doesn't exist, to show the fallback to type

Unfortunately that gives us 7 :(

square
inside               
url('path/to/image.svg')
lower-roman inside
georgian inside url('path/to/image.svg')
georgian inside url('i-dont-exist.svg')
none

I'm really not sure which of these to omit. Perhaps lower-roman inside since that's implicitly covered in the 3 value syntax?

wbamberg · 2018-02-07T19:18:30Z

It would be worth thinking of how to show the effect of inside: it's not clear in this example as it stands. I did think about having a light background-color for the li items, and giving them a little horizontal padding to accentuate it. This shows that outside is outside the element, and inside is inside it:

Inside:

Outside:

Note though that this apparently does not work when you have 50% width on the ul.

wbamberg · 2018-02-07T19:22:19Z

I think it would be nice to make the actual list more concrete and fun, and give it an intro line saying what it's a list of.

Like:

Moons of Jupiter:

Callisto

Ganymede

Io

Europa

Or something else, but you get the idea :).

wbamberg · 2018-02-07T19:54:11Z

I don't think the Firefox icon looks good when it is scaled down this far. It would be worth trying to find a simpler SVG image (like a star or something), if you can find one that's public domain.

wbamberg

@helmutgranda , thanks for the example! I did have a few comments, I hope they are helpful. Please feel free to let me know if you don't agree with them or are finding them hard to address :).

helmutgranda · 2018-02-08T05:23:30Z

Thanks for the feedback @wbamberg, I have committed some changes based on your recommendations.

schalkneethling

Couple of minor tweaks left over. Thanks so much @helmutgranda

schalkneethling · 2018-02-08T08:30:17Z

live-examples/css-examples/lists-and-counters/list-style.css

+
+.output section li {
+    background: #f1f8ff;
+}


Nit: missing empty line at the end of the file.

schalkneethling · 2018-02-08T08:31:14Z

live-examples/css-examples/lists-and-counters/list-style.html

@@ -0,0 +1,61 @@
+<section id="example-choice-list" class="example-choice-list large" data-property="list-style">
+


Super nit: For consistency with other files, please remove this empty line.

schalkneethling · 2018-02-08T08:31:21Z

live-examples/css-examples/lists-and-counters/list-style.html

+                <span class="visually-hidden">Copy to Clipboard</span>
+        </button>
+    </div>
+


Super nit: For consistency with other files, please remove this empty line.

schalkneethling · 2018-02-08T08:32:16Z

media/examples/rocket.svg

+   </g>
+  </g>
+ </g>
+</svg>


Nit: missing empty line at end of file. I would also suggest running this through https://jakearchibald.github.io/svgomg/

Thanks! I didn't know about this tool.

helmutgranda · 2018-02-08T14:30:09Z

I think this should be good to go now, all recommendations from @wbamberg and @schalkneethling in place.

danielhickman

Your other PR had these removed but maybe these changes should be made too on this PR.

danielhickman · 2018-02-08T16:41:34Z

live-examples/css-examples/lists-and-counters/list-style.html

@@ -0,0 +1,58 @@
+<section id="example-choice-list" class="example-choice-list large" data-property="list-style">
+    <div class="example-choice" initial-choice="true">
+        <pre><code id="example_one" class="language-css">list-style: square;</code></pre>


Looks like the id="example_one" boilerplate was missed.

Well, in reality it was not missed. The boilerplate update was introduced after this PR ;) and I was not sure if the team was going to request updates to all PRs, either way I will be cleaning that up and submitting an update.

Yeah, totally. Awesome!

schalkneethling

r+ from me, but I will wait for @wbamberg to review before merging. Thanks @helmutgranda

wbamberg

Looks great, @helmutgranda , thanks so much for making those changes.

One very minor nit, but I'll approve this anyway.

One question though: I love the rocket icon, I think it's a great choice. Can you confirm that it's in the public domain (e.g. https://creativecommons.org/share-your-work/public-domain/) and we are therefore allowed to use it? Thanks!

wbamberg · 2018-02-08T19:35:57Z

live-examples/css-examples/lists-and-counters/list-style.html

+        </button>
+    </div>
+</section>
+<div id="output" class="output large hidden">


nit: we should have an empty line between /section and div.

Hey @wbamberg, I added the empty line.

In regards to the icon, I found it here:
https://openclipart.org/detail/266929/rocket
// updated URL for better source of rocket image.

Let me know if that works for this project or if I need to look somewhere else.

That's perfect!

wbamberg · 2018-02-08T20:11:05Z

@danielhickman 's review is still outstanding, but I think the latest changes have removed the boilerplate, so I'm merging this :).

danielhickman · 2018-02-08T20:16:01Z

Yeah the changes were made. Was there something I should've done or should do in the future to mark my review as "approved"? I only thought I had suggesting power since I don't have push access.

wbamberg · 2018-02-08T21:52:35Z

Was there something I should've done or should do in the future to mark my review as "approved"? I only thought I had suggesting power since I don't have push access.

I'm not sure, but the "Reviewers" section of this PR looks like:

...as if it wants your review approval. When I have requested changes I see something like this:

...and can hit "approve changes" (or "Review changes->Approve" in https://github.com/mdn/interactive-examples/pull/547/files). But I don't know if you can, too.

In general I'd rather not merge stuff when someone else has outstanding comments, although your 'approval' could just be a comment saying "yes, that's fine" :). But in this case your comment seemed clear and I was pretty sure it had been addressed.

danielhickman · 2018-02-08T22:36:30Z

Okay, that's helpful. I'll look out for those options in the future or leave a comment. Thanks.

@helmutgranda

* upstream/master: Adding list-style css example. (mdn#547) Flex examples (mdn#558) Add column examples (mdn#549) Remove CSS example id attributes (mdn#556) Add various text examples (mdn#545) Add contribution item (mdn#552) Adds @helmutgranda as contributor (mdn#550) Adding border radius example with recommended changes. (mdn#546) Add `quotes` example (mdn#543)

Adding list style css example.

df64b12

wbamberg self-requested a review February 7, 2018 01:29

schalkneethling suggested changes Feb 7, 2018

View reviewed changes

Increased font size and custom stylesheet.

1fd0bab

wbamberg suggested changes Feb 7, 2018

View reviewed changes

wbamberg mentioned this pull request Feb 7, 2018

CSS examples: example needed for list-style-type #483

Closed

danielhickman mentioned this pull request Feb 7, 2018

Remove CSS example id attributes #556

Merged

4 tasks

Changing list elements and adding new images for bullet points.

fef557d

schalkneethling suggested changes Feb 8, 2018

View reviewed changes

Cleaning up code, removing spaces and minifying SVG.

d39cd78

danielhickman suggested changes Feb 8, 2018

View reviewed changes

schalkneethling approved these changes Feb 8, 2018

View reviewed changes

Updating document to match boilerplate

909eb1f

wbamberg approved these changes Feb 8, 2018

View reviewed changes

Fixing spacing issues.

905b39a

wbamberg merged commit 7fb171d into mdn:master Feb 8, 2018

helmutgranda deleted the add-list-style-css-example branch February 8, 2018 20:13

wbamberg mentioned this pull request Feb 8, 2018

CSS examples: example needed for list-style #484

Closed

		@@ -0,0 +1,61 @@
		<section id="example-choice-list" class="example-choice-list large" data-property="list-style">

+                 </g>
+                </g>
+               </g>
+              </svg>

                
                    No newline at end of file

Adding list-style css example. #547

Adding list-style css example. #547

Uh oh!

Conversation

helmutgranda commented Feb 6, 2018

Uh oh!

schalkneethling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbamberg commented Feb 7, 2018

Uh oh!

wbamberg commented Feb 7, 2018

Uh oh!

wbamberg commented Feb 7, 2018

Uh oh!

wbamberg commented Feb 7, 2018

Uh oh!

wbamberg commented Feb 7, 2018

Uh oh!

wbamberg commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbamberg left a comment

Choose a reason for hiding this comment

Uh oh!

helmutgranda commented Feb 8, 2018

Uh oh!

schalkneethling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helmutgranda commented Feb 8, 2018

Uh oh!

danielhickman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schalkneethling left a comment

Choose a reason for hiding this comment

Uh oh!

wbamberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helmutgranda Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbamberg commented Feb 8, 2018

Uh oh!

danielhickman commented Feb 8, 2018

Uh oh!

wbamberg commented Feb 8, 2018

Uh oh!

danielhickman commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

wbamberg commented Feb 7, 2018 •

edited

Loading

helmutgranda Feb 8, 2018 •

edited

Loading