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

Fix breaklines #107

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Fix breaklines #107

wants to merge 15 commits into from

Conversation

smaudet
Copy link

@smaudet smaudet commented Apr 2, 2016

I would like to submit this for consideration.

Currently, if you use this tool, you have to do a lot of cleanup work to get br to work right. br doesn't expand correctly in jade atm, this proposed fix adds a mixin to the top of jade files with a doctype that replaces calls to br with +bbr(), and inserts a <br /> into the code.

Spacing is odd, however there are already issues in both jade and this project which prevent it from being perfect.

There is a test file, as well as existing tests have been modified to update the expectations.

@smaudet
Copy link
Author

smaudet commented Apr 2, 2016

I'm trying to make the travis build pass, something broke with your travis.

I think your travis build is hypersensitive to line endings perhaps.

@donpark
Copy link
Owner

donpark commented Apr 3, 2016

Looks like you've put a lot of work into this PR. But I am going to defer merging it because, at least to me, it seems overly elaborate and opinionated. My being unaware of seriousness of br related issues and shortcomings may have also contributed. Sorry.

Would you like me to link to your branch from the README to help users who share your needs?

@smaudet
Copy link
Author

smaudet commented Apr 3, 2016

@donpark what part of it seems elaborate and opinionated? I'm fine with an update in your readme to point to this branch.

Seriousness of the <br /> issue is that <br> doesn't always work. It should...but I've come across cases where it didn't, and even according to standard specs, <br /> would work where <br> would not when using non html5, or some form of xhtml. Perhaps there should be a flag that allows toggling this feature?

In making the tests/travis builds work, I noticed somethings that were broken in other areas:

  • Doesn't test properly on windows - added .gitattributes to fix that
  • Updated mocha/nodejs to something current (0.10 is ancient)
  • Updated coffeescript to something recent
  • Updated mocha.opts so that nodejs doesn't throw up an ugly build error.

If you're not happy with this feature I might refactor those changes out of here into another branch and PR that at least, so that other users who might share my needs don't have the same problems. Let me know if you think the changes in the areas outlined above merit another separate PR.

Pulling master clean, it also looks like testing is working now after I disabled my global autocrlf = true, so either on windows you may simply need to add a local .gitconfig that turns it off, or put something in the README warning users to change this in their global .gitconfig (which is in general inconvenient for windows users).

@donpark
Copy link
Owner

donpark commented Apr 3, 2016

Well, the thing is I don't use br in my apps. While the tag is useful sometimes while hacking to try this and that to find a workaround for CSS issues, I've never ran across a use-case where it made good design sense nor the only solution.

FYI, I added link to your branch to README.md.

@smaudet
Copy link
Author

smaudet commented Apr 3, 2016

Well, that's reasonable I suppose.

I'd like to see this tool being able to do round-trip parsing, i.e. html2jade and jade2html (just using jade), with as close parity as possible.

Of course this is not perfectly possible because of things like mixins/loops, but for simple templates this should be very possible (treat simple mixin like html pages with 'default' parameters, maybe compile some sort of .map file that can hold some information about the loop transformations).

You may be wondering why I think this might be useful - generally I suspect your intended workflow is:

  1. transform legacy html to jade
  2. transform jade to html
  3. never run the html2jade tool again

But if you have someone new to the jade language, it could be very useful to have some sort of approximation like this, or for tools that might do real-time html/jade transformation.

So, from that perspective, any <br> tag that might not work, or other problems using this tool might produce, are issues. <br> was the biggest one which I had seen.

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.

2 participants