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

Fix issue #475: diet doesn't allow boolean attributes without value in H... #512

Merged
merged 3 commits into from Feb 12, 2014

Conversation

Projects
None yet
3 participants
@nazriel
Contributor

nazriel commented Feb 9, 2014

...TML5

Again, not sure if it is the best approach.
Is DietCompiler the right place for isHTML5 flag?

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Feb 9, 2014

Contributor

Travis CI failure unrelated.
UT passes on my PC.

Contributor

nazriel commented Feb 9, 2014

Travis CI failure unrelated.
UT passes on my PC.

@@ -605,11 +607,13 @@ private struct DietCompiler {
private void buildDoctypeNodeWriter(OutputContext output, string ln, size_t j, int level)
{
skipWhitespace(ln, j);
isHTML5 = false;
string doctype_str = "!DOCTYPE html";
switch (ln[j .. $]) {
case "5":
case "":

This comment has been minimized.

@s-ludwig

s-ludwig Feb 10, 2014

Member

"html" also needs to be added as a case here (this is the recommended value to define HTML 5 now in Jade). I've missed that in the original pull request.

@s-ludwig

s-ludwig Feb 10, 2014

Member

"html" also needs to be added as a case here (this is the recommended value to define HTML 5 now in Jade). I've missed that in the original pull request.

This comment has been minimized.

@nazriel

nazriel Feb 10, 2014

Contributor

@s-ludwig right.

Although I think "html" case will need additional test

if (ln.length > 0) goto case default;
else isHTML5 = true;

Jade has few predefined shortcuts but it also allows to specify whatever you want in doctype, for example this is valid:

doctype html moofoo

But I guess it isn't html5 anymore 👻

EDIT:
actually nevermind. I mistaken tag with doctype. Everything is fine :)

@nazriel

nazriel Feb 10, 2014

Contributor

@s-ludwig right.

Although I think "html" case will need additional test

if (ln.length > 0) goto case default;
else isHTML5 = true;

Jade has few predefined shortcuts but it also allows to specify whatever you want in doctype, for example this is valid:

doctype html moofoo

But I guess it isn't html5 anymore 👻

EDIT:
actually nevermind. I mistaken tag with doctype. Everything is fine :)

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2014

Member

I think this is good to go, except that the flag also needs to be passed to secondary DietCompiler instances (used for blocks/extensions and includes), so an additional constructor argument may be needed.

Member

s-ludwig commented Feb 10, 2014

I think this is good to go, except that the flag also needs to be passed to secondary DietCompiler instances (used for blocks/extensions and includes), so an additional constructor argument may be needed.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2014

Member

BTW, I'll update travis to use the latest DUB preview after I've released that later today, so that the random errors hopefully vanish.

Member

s-ludwig commented Feb 10, 2014

BTW, I'll update travis to use the latest DUB preview after I've released that later today, so that the random errors hopefully vanish.

@UplinkCoder

This comment has been minimized.

Show comment
Hide comment
@UplinkCoder

UplinkCoder Feb 10, 2014

Contributor

isn't OutputContext the place for such a flag ?

Contributor

UplinkCoder commented Feb 10, 2014

isn't OutputContext the place for such a flag ?

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Feb 10, 2014

Contributor

@s-ludwig updated.

I noticed that we can do now even more "optimizations".
For example

input(type="text",name="foo")

is rewritten as

<input type="text" name="foo"/>

I guess we can get rid of / in HTML5

Contributor

nazriel commented Feb 10, 2014

@s-ludwig updated.

I noticed that we can do now even more "optimizations".
For example

input(type="text",name="foo")

is rewritten as

<input type="text" name="foo"/>

I guess we can get rid of / in HTML5

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2014

Member

isn't OutputContext the place for such a flag ?

That's actually a good idea. I didn't consider it, because I thought of it more like a very low level facility, but it already contains things like the node stack, so this makes a lot of sense.

Member

s-ludwig commented Feb 10, 2014

isn't OutputContext the place for such a flag ?

That's actually a good idea. I didn't consider it, because I thought of it more like a very low level facility, but it already contains things like the node stack, so this makes a lot of sense.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2014

Member

I guess we can get rid of / in HTML5

This is already supposed to be done and should result in just <input type="text" name="foo"> (search for is_singular_tag), but I guess that this code now needs to be fixed to only do this for singular tags of the actually selected !DOCTYPE.

Member

s-ludwig commented Feb 10, 2014

I guess we can get rid of / in HTML5

This is already supposed to be done and should result in just <input type="text" name="foo"> (search for is_singular_tag), but I guess that this code now needs to be fixed to only do this for singular tags of the actually selected !DOCTYPE.

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Feb 10, 2014

Contributor

@s-ludwig updated to use OutputContext (actually I was thinking about it too yesterday when making this PR).

Well, it was making <input ... /> for me when testing... now it will pass on / when doing html5

Contributor

nazriel commented Feb 10, 2014

@s-ludwig updated to use OutputContext (actually I was thinking about it too yesterday when making this PR).

Well, it was making <input ... /> for me when testing... now it will pass on / when doing html5

@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Feb 10, 2014

Contributor

@s-ludwig updated (See also outdated diff in-line commnet) ;)

Contributor

nazriel commented Feb 10, 2014

@s-ludwig updated (See also outdated diff in-line commnet) ;)

s-ludwig added a commit that referenced this pull request Feb 12, 2014

Merge pull request #512 from nazriel/issue475
Fix issue #475: diet doesn't allow boolean attributes without value in H...

@s-ludwig s-ludwig merged commit 69ccaf8 into vibe-d:master Feb 12, 2014

1 check passed

default The Travis CI build passed
Details
@nazriel

This comment has been minimized.

Show comment
Hide comment
@nazriel

nazriel Feb 12, 2014

Contributor

@s-ludwig thanks!
Also please check your mailbox in free moment :) I've sent you a message.

Contributor

nazriel commented Feb 12, 2014

@s-ludwig thanks!
Also please check your mailbox in free moment :) I've sent you a message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment