Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

Update the html to match the latest code #45

Merged
merged 5 commits into from Mar 6, 2014

Conversation

vicb
Copy link
Contributor

@vicb vicb commented Mar 4, 2014

see it online @ http://vicb-angulardart.appspot.com/

The first commit has the kind of magics I like: it allows anybody to easily update the code in the html (ie it removes the burden of having to escape the HTML entities).

The second commit is the "real" update. Hopefully I haven't miss anything... anyway it's now much easier for anybody to update.

@kwalrath
Copy link
Contributor

kwalrath commented Mar 4, 2014

I took a quick look, and noticed that in http://vicb-angulardart.appspot.com/tutorial/03-ch01-creating-your-first-app.html, the "two" script tags (which are 3 in https://angulardart.org/tutorial/03-ch01-creating-your-first-app.html, oops) are just 1:

<script src="packages/shadow_dom/shadow_dom.min.js">

The application/dart and text/javascript tags are missing. So it looks like the new script doesn't deal with <script> tags properly. I'm guessing the </script> tag from the first line ended the <pre>-equivalent section.

Even before I found that it made me a bit nervous to use a <script> tag to mark <pre>-equivalent sections. Is this technique used anywhere else?

@kwalrath
Copy link
Contributor

kwalrath commented Mar 4, 2014

PS: This is very cool work, and anything that makes it easier to update code samples is a good thing!

@vicb
Copy link
Contributor Author

vicb commented Mar 4, 2014

about the script technique don't worry, John Resig (the creator of jQuery) was one of the first to promote this: http://ejohn.org/blog/javascript-micro-templating/.

You are right on the fact that <script> tags could not be nested... one solution could be to replace them by [script] - can not think of anything better at the time.

@kwalrath
Copy link
Contributor

kwalrath commented Mar 4, 2014

We could also leave <script> tags in <pre>. (Either way, we should have a helpful comment for future editors, so they don't break this in the future.)

@kwalrath
Copy link
Contributor

kwalrath commented Mar 4, 2014

PS: That link does make me feel better about using <script> in this way. :)

@vicb
Copy link
Contributor Author

vicb commented Mar 4, 2014

We could also leave <script> tags in <pre>

Simple is great! I'll update for this tomorrow (provided we are not using other scripts tags somewhere else)

@kwalrath
Copy link
Contributor

kwalrath commented Mar 4, 2014

It looks like only ch01 & ch07 have <script> tags in the code snippets.

@vicb
Copy link
Contributor Author

vicb commented Mar 5, 2014

thanks for the hint, changes commited.

@kwalrath
Copy link
Contributor

kwalrath commented Mar 5, 2014

Could you update http://vicb-angulardart.appspot.com/ so I can do a visual diff? Thanks!

@vicb
Copy link
Contributor Author

vicb commented Mar 5, 2014

it's live now

@kwalrath
Copy link
Contributor

kwalrath commented Mar 5, 2014

In http://vicb-angulardart.appspot.com/tutorial/07-ch05-filter-service.html I happened to notice a couple of code snippets (inside the "Built in filters" section) that don't have enough space above them. E.g., the upper border of the snippet that starts <input id="name-filter" type = "text" is right up against the last line of the previous paragraph. The same is true of the next snippet.

This doesn't happen everywhere, but where it does, it's much more noticeable when the last line of the previous paragraph contains some code font. E.g., I didn't notice this problem above String nameFilterString = ""; but upon closer inspection that snippet has too little space above it.

Perhaps you removed a line above these snippets? Or is there a style change we need to make?

Oh, and... This is new to your version. (I was visually comparing with the old version.)

@vicb
Copy link
Contributor Author

vicb commented Mar 5, 2014

That was definitely an issue with the markup, it should be fixed now.

@vicb
Copy link
Contributor Author

vicb commented Mar 5, 2014

thanks for catching it !

@@ -223,81 +223,81 @@ <h3 id="encapsulating-view-logic-into-components">
<div class="col-md-6">
<h3>index.html (previous Chapter)</h3>

<pre class="prettyprint">
&lt;div recipe-book&gt;
<script type="template/dart">
Copy link
Contributor

Choose a reason for hiding this comment

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

This example probably needs to stay as a <pre>, since it formats the code (using <strong>, e.g.).

At least, I assume the formatting is intentional...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch again !

The formatting is indeed intentional.

I would prefer loosing the formatting here over switching back to escaped entities but I'll do whatever your prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have strong feelings either way.

-k-

On Wed, Mar 5, 2014 at 8:24 AM, Victor Berchet notifications@github.comwrote:

In app/tutorial/08-ch06-view.html:

@@ -223,81 +223,81 @@



index.html (previous Chapter)

-


-<div recipe-book>
+<script type="template/dart">

Nice catch again !

The formatting is indeed intentional.

I would prefer loosing the formatting here over switching batch to escaped
entities but I'll do whatever your prefer.


Reply to this email directly or view it on GitHubhttps://github.com//pull/45/files#r10306023
.

@kwalrath
Copy link
Contributor

kwalrath commented Mar 5, 2014

Once you fix the few things I commented on, this looks great to me.

@vicb
Copy link
Contributor Author

vicb commented Mar 6, 2014

  • removed the <strong> formatting to keep script tags (easier to update),
  • fixed <script class="prettyprint">
  • changed template/dart to template/code so it could be used everywhere (no only dart)

Will push the commit & update the live version in the coming minutes.

@vicb
Copy link
Contributor Author

vicb commented Mar 6, 2014

Done and thanks again for the very good feedback !

@kwalrath
Copy link
Contributor

kwalrath commented Mar 6, 2014

So close!

@vicb
Copy link
Contributor Author

vicb commented Mar 6, 2014

I think I'm done now :)

@kwalrath
Copy link
Contributor

kwalrath commented Mar 6, 2014

lgtm! Thanks for making the docs better (and for bearing through the review process :).

kwalrath added a commit that referenced this pull request Mar 6, 2014
Update the html to match the latest code
@kwalrath kwalrath merged commit 5fd419f into dart-archive:master Mar 6, 2014
@vicb vicb deleted the updates branch March 6, 2014 16:56
@vicb
Copy link
Contributor Author

vicb commented Mar 6, 2014

Cool! Many thanks for reviewing and catching so many errors !

@kwalrath
Copy link
Contributor

kwalrath commented Mar 6, 2014

Attention to detail is my gift and curse. :) You're very welcome.

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

Successfully merging this pull request may close these issues.

None yet

2 participants