Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Ng-switch commentary has unnecessary white space #14549

Closed
ivomju opened this issue May 2, 2016 · 8 comments
Closed

Ng-switch commentary has unnecessary white space #14549

ivomju opened this issue May 2, 2016 · 8 comments

Comments

@ivomju
Copy link
Contributor

ivomju commented May 2, 2016

When creating ng-switch-when end commentary, because no comment is passed an unnecessary white space is generated.

Before:

<!-- end ngSwitchWhen: -->

Now:

<!-- end ngSwitchWhen:  -->

To prevent that we could do something like this:

compile.$$createComment = function(directiveName, comment) {
      var content = '';

      if (debugInfoEnabled) {
        content = ' ';
        content += (directiveName || '');
        content += ':';
        content += comment ? ' ' + comment : '';
        content += ' ';
      }

      return window.document.createComment(content);
    };
@arashkbanan
Copy link

ایول

@Narretz
Copy link
Contributor

Narretz commented May 2, 2016

Thanks for the report. This looks pretty inconsequential, but we might incorporate this change if your open a PR with this.

@Narretz
Copy link
Contributor

Narretz commented May 2, 2016

Anyway, does this cause you any problems in your app?

@ivomju
Copy link
Contributor Author

ivomju commented May 2, 2016

Yep, this broke some unit tests because we've tested the html output. I will open a PR then, thanks.

@ivomju
Copy link
Contributor Author

ivomju commented May 2, 2016

But as you said, this is inconsequential with no big impact.

@Narretz
Copy link
Contributor

Narretz commented May 2, 2016

I'm curious, why would you test the output of ngSwitch in a unit test? Shouldn't you test for presence / absence of non-comment nodes? Because the comments added by angular directives are implementation details.

@ivomju
Copy link
Contributor Author

ivomju commented May 2, 2016

Summing this up:

  • Very old tests where we define our directive scope, compile it and checked the html output
  • This is on our backlog team to refactor but, unfortunately, It's not a priority.

@ivomju
Copy link
Contributor Author

ivomju commented May 2, 2016

Also, this HTML validation shouldn't be done in unit test IMO, automated tests exists for something.

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

No branches or pull requests

3 participants