Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some changes to improve readability. #4325

Merged
merged 3 commits into from Oct 5, 2013
Merged

Some changes to improve readability. #4325

merged 3 commits into from Oct 5, 2013

Conversation

damianavila
Copy link
Member

As the title says... some changes in spaces to improve readability and to keep an uniform style in the htm-based templates.

@@ -6,7 +6,7 @@
<html>
<head>

<meta charset="utf-8" />
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="chrome=1">

<meta name="apple-mobile-web-app-capable" content="yes" />
Copy link
Member

Choose a reason for hiding this comment

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

want to remove the / here too ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We remove from here... or we also put it in the full_html template... no preference, just I want to have the same in both templates.

Copy link
Member

Choose a reason for hiding this comment

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

For html 5 / is syntactic sugar, for meta which is is a void element I just think then we should keep the same thig everywhere either never / or always /. Keeping / would increase compatibility with xhtml (I think). but the issue can be pretty subtle

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the / to both templates ;-)

@Carreau
Copy link
Member

Carreau commented Oct 2, 2013

Small comment, but +1 globally.

@@ -5,8 +5,9 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
<title>[{{nb.metadata.name}}]</title>
Copy link
Member

Choose a reason for hiding this comment

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

This <title> tag seems to have gone missing - was that deliberate?

Copy link
Member

Choose a reason for hiding this comment

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

yes; nb.metadata.name is empty now...

Copy link
Member

Choose a reason for hiding this comment

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

But that doesn't mean we shouldn't set the title, it just means we were setting it wrong. Isn't there something in resources that we can use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe here:

resources['metadata']['name'] = notebook_name

Copy link
Member

Choose a reason for hiding this comment

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

OK, I missed that it was reintroduces when file was read...

@Carreau
Copy link
Member

Carreau commented Oct 4, 2013

Need rebases also.

@damianavila
Copy link
Member Author

Rebased and added titles to full_html and slides templates...

@takluyver
Copy link
Member

Looks good to me. @Carreau , feel free to merge if there's nothing else you want to check.

@Carreau
Copy link
Member

Carreau commented Oct 5, 2013

Nothing to add, thanks, merging.

Carreau added a commit that referenced this pull request Oct 5, 2013
Some changes to improve readability.
@Carreau Carreau merged commit f3d4ed1 into ipython:master Oct 5, 2013
@damianavila damianavila deleted the cleaning branch October 5, 2013 10:20
@damianavila
Copy link
Member Author

Thanks!

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Some changes to improve readability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants