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

Conditional Comments #1345

Closed
gisu opened this issue Dec 26, 2013 · 17 comments
Closed

Conditional Comments #1345

gisu opened this issue Dec 26, 2013 · 17 comments

Comments

@gisu
Copy link

gisu commented Dec 26, 2013

Any Conditional Comments didn't work with JADE 1.0.

//if lt IE 8
  link(rel='stylesheet', href='style-ie8.css')

Gives

<!-- if lt IE 8link(rel='stylesheet', href='components/css/style-ie8.css')
-->

Is this a bug, is this functionality dropped its a new secrete option that i have to activate?

@ForbesLindesay
Copy link
Member

This feature has been deliberately dropped as it created complicated confusing behaviour by forcing us to parse the contents of a comment. Instead you should now just write:

<!--[if lt IE 8]>
link(rel='stylesheet', href='style-ie8.css')
<![endif]-->

Jade will ignore any line beginning with < and because it matches the html syntax it can be instantly identified by anyone reading it.

@louh
Copy link

louh commented Jan 7, 2014

When I just use conditional comments as plain text it throws an 'unexpected token “indent”' error in Express.

EDIT. Workaround was to include the contents of it as an html file rather than as a jade-parsed file.

@ForbesLindesay
Copy link
Member

Note that the body of the conditional comment must not be indented.

@ghost ghost mentioned this issue Jan 8, 2014
5 tasks
@notslang
Copy link
Contributor

notslang commented Jan 9, 2014

can we revert this?

  • that new syntax is ugly. jade shouldn't be ugly
  • this breaks any template that uses conditional comments (without any deprecation warning. nothing. just broken)
  • it doesn't make sense to have uncompiled Jade anywhere in the output. for example, this breaks ResponsiveComments if used with jade

@kennethormandy
Copy link

I have to agree with @slang800, I would like to see this reverted and, in the same vein, #1374 accepted.

Sorry, I realise it must be frustrating to have people like me randomly show up and complain about this stuff, I know it was planned for a while. Personally, I just never really discover the new stuff until the new release is out! I’m going to try and pay more attention in the future.

@ghost
Copy link

ghost commented Jan 9, 2014

@slang800 Why don't you just use a custom filter for the ResponsiveComments scenario? You'd make the syntax highlighter happier.

@ForbesLindesay
Copy link
Member

You can easilly have html in comments if you need it by simply adding <!-- and -->. Since comments are an extremely common use case of comments, and in-lining html that you don't wish to be parsed is a pretty rare use case for comments. In fact there's a much better way to do that and it's a script tag with a type that specifies it as html. As for conditional comments, they are/should be a pretty rare use case and nothing jade does will work for a lot of people using them because you can't represent:

<!--[if IE]>
   <html class="ie">
<![endif]-->
<![if !IE]>
   <html class="not-ie">
<![endif]>
</html>

I got a lot of support queries from people who either wanted to write that, and were frustrated that they couldn't, or wanted to write comments, and were frustrated that invalid syntax in their comments broke their templates. The costs outweighed the benefits vastly so the feature was removed. I have no intention of bringing it back. The syntax for conditional comments are ugly for a reason: it's an ugly thing to do.

@cspotcode
Copy link
Contributor

How about a nice mixin? I'm not sure if it should be included in Jade by default since it adds if-ie to the mixin namespace. On the other hand, that can always be shadowed by a user's mixins so it shouldn't break anything.

mixin if-ie(condition, showInNonIeBrowser)
  - if(showInNonIeBrowser == null) showInNonIeBrowser = condition == "!IE";
  != "<!--[if " + condition + "]>"
  if showInNonIeBrowser
    != "-->"
  block
  if showInNonIeBrowser
    != "<!--"
  != "<![endif]-->"

+if-ie("lt IE 8") Your browser is too old!
+if-ie("!IE") You're not using IE!

@ForbesLindesay
Copy link
Member

That seems like a really nice elegant solution. It's great to see this feature can be implemented so neatly in user space. I do not want to add a standard library of mixins to jade (this would just be the beginning). What I would like though, is a way to import jade from npm. This would let people publish an npm module jade-conditional-comments that just exposes that mixin. Needs work to flesh out into a full concept though.

@harrypujols
Copy link

This thing created a mess in my legacy jade files. Now how do I do a conditional like this (as in the html5 Boilerplate)?

 <!-- [if gt IE 8] <!-->
<html class="no-js">
<!-- <![endif]-->

Previously, this is how it was done:

// [if gt IE 8] <!
html.no-js
  // <![endif]

@cspotcode
Copy link
Contributor

I use != to specify some raw HTML for Jade to output. I like it because it's simple and easy to understand, and I can live with a tiny bit of messiness.

!= "<!-- [if gt IE 8] <!-->"
html.no-js
    != "<!-- <![endif]-->"

Full HTML5 Boilerplate example here:
https://github.com/cspotcode/html5-boilerplate/blob/custom/index.jade#L2-7

@harrypujols
Copy link

That's a good solution @cspotcode
For that particular piece of code, it will do the work.
You may want to check your HTML5 Boilerplate example. The conditionals at the start were closing the HTML files. This is how they compiled on my document:

<!DOCTYPE html>
<!--[if lt IE 7]>
<html class="no-js lt-ie9 lt-ie8 lt-ie7"> </html>
<![endif]-->
<!--[if IE 7]>
<html class="no-js lt-ie9 lt-ie8"> </html>
<![endif]-->
<!--[if IE 8]>
<html class="no-js lt-ie9"> </html>
<![endif]-->
<!-- [if gt IE 8] <!-->
<html class="no-js">
  <!-- <![endif]-->

@ForbesLindesay
Copy link
Member

Yep, you have to use raw html to do that, rather than jade. Jade has never supported conditional comments around only the start, but not the end, of a tag. What jade does support though, is inline html. As long as the line starts with < it will be ignored. So just copy and paste that part of the html5 boilerplate:

doctype html
<!--[if lt IE 7]>      <html class="no-js lt-ie9 lt-ie8 lt-ie7"> <![endif]-->
<!--[if IE 7]>         <html class="no-js lt-ie9 lt-ie8"> <![endif]-->
<!--[if IE 8]>         <html class="no-js lt-ie9"> <![endif]-->
<!--[if gt IE 8]><!--> <html class="no-js"> <!--<![endif]-->
head
  meata(charset='utf-8')
  //etc.etc.
body
  p Hello world! etc. etc.
</html>

Alternatively don't, because conditional comments are a terrible idea and are even being removed from the html5 boilerplate soon (see h5bp/html5-boilerplate@13f17a7)

@cspotcode
Copy link
Contributor

@harrypujols Are you referring to the example I linked? (https://github.com/cspotcode/html5-boilerplate/blob/custom/index.jade#L2-7)
It compiles fine for me.

Or were you talking about something else?

@louh
Copy link

louh commented Mar 20, 2014

Alternatively don't, because conditional comments are a terrible idea and are even being removed from the html5 boilerplate soon (see h5bp/html5-boilerplate@13f17a7)

Particularly those conditional comments, since they're super hacky, and all they do is attach classes for CSS. If you don't make heavy use of those classes, you can do away with them easily. If you must have IE7, IE8, etc specific styles, set up alternate stylesheets with conditional comments that don't hack the HTML tags.

I went through this a short time ago and realized getting Jade to behave with conditional comment hacks was not a good use of my time.

@harrypujols
Copy link

@cspotcode yes, it was the example provided in your link that was producing the closed html tags. I don't know why it compiles different. The way it compiles correctly for me is doing it this way:

<!--[if lt IE 7]>
<html class="no-js lt-ie9 lt-ie8 lt-ie7">
<![endif]-->
<!--[if IE 7]>
<html class="no-js lt-ie9 lt-ie8">
<![endif]-->
<!--[if IE 8]>
<html class="no-js lt-ie9">
<![endif]-->
!= "<!--[if gt IE 8]><!-->"
html.no-js
  != "<!--<![endif]-->"

@louh since I am adapting the new way of doing conditional comments to sites that were already using them, it's already too late to get rid of them.

@GeorgeWL
Copy link

GeorgeWL commented Jun 6, 2018

@louh @ForbesLindesay

You're missing out edge cases over here.

Nodemailer and other Mail-sender packages can use Pug Templates to generate the HTML used for E-mails and since no major email clients follow even HTML 4 spec, never mind HTML 5, conditionals are actually a requirement to hack your way around the god damn stupidity that many email clients have.

@pugjs pugjs locked as resolved and limited conversation to collaborators Jun 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants