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

Popover breaks when multiline template literal is used. #233

Closed
fleck opened this issue Aug 10, 2018 · 13 comments
Closed

Popover breaks when multiline template literal is used. #233

fleck opened this issue Aug 10, 2018 · 13 comments
Assignees
Labels
enhancement potential improvement

Comments

@fleck
Copy link

fleck commented Aug 10, 2018

When I initialize a popover with a multiline string:

      const template = `
        <div class="popover" role="tooltip">
          <div class="arrow"></div>
          <h3 class="popover-title"></h3>
          <div class="popover-content"></div>
        </div>` ;

      this.popover = new Popover(ribbonElement, {template, placement: 'left'});

I get the error 'Cannot read property 'offsetWidth' of null' from this line of code:
arrowWidth = arrow[offsetWidth]; arrowHeight = arrow[offsetHeight];

@thednp
Copy link
Owner

thednp commented Aug 10, 2018

Perhaps you didn't NAME the option?

this.popover = new Popover(ribbonElement, {template: template, placement: 'left'});

@thednp thednp closed this as completed Aug 10, 2018
@fleck
Copy link
Author

fleck commented Aug 10, 2018

This is in the chrome console which supports property shorthand. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#Property_definitions So our 2 examples are functionally the same.

@fleck
Copy link
Author

fleck commented Aug 10, 2018

here's a fiddle reproducing the bug https://jsfiddle.net/jonfleck/obskwpta/14/

@thednp
Copy link
Owner

thednp commented Aug 10, 2018

I will have a look at this asap. Thanks for reporting.

@thednp thednp reopened this Aug 10, 2018
@thednp
Copy link
Owner

thednp commented Aug 11, 2018

OK so I've done some changed to your fiddle example, it's set (and I think it was) to pure JS, and it's working as expected:

  • I replaced &#768; (accent grave) with ' (single quotation)
  • I properly wrapped each row in ' (single quotation)
  • I added a title and desc to the template
  • all new lines are concatenated properly into the template string via +

All looks good to me now. The previous notation wasn't proper JavaScript, the &#768; (accent grave) is meant for something else and not contain strings. So we're now closing this.

@thednp thednp closed this as completed Aug 11, 2018
@thednp thednp added the invalid not related or critical label Aug 11, 2018
@fleck
Copy link
Author

fleck commented Aug 11, 2018

The backtick is valid syntax. It was introduced to JavaScript to make templating easier. From the docs: “The default function just concatenates the parts into a single string.” https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

@thednp
Copy link
Owner

thednp commented Aug 11, 2018

If that's the case, you're free to do any contribution you consider fit to solve that particular issue. I still think this backtick support isn't fully implemented in our browsers, or else it should work, nothing to do with our component at all.

@felschr
Copy link

felschr commented Apr 30, 2019

This issue comes up for me when the string starts with a line break.
So OP's issue could be fixed by using the following template literal instead:

const template = `<div class="popover" role="tooltip">
    <div class="arrow"></div>
    <h3 class="popover-title"></h3>
    <div class="popover-content"></div>
  </div>`;

Alternatively using .trim() on the string would fix the issue as well.

@thednp thednp reopened this Jun 3, 2019
@thednp thednp added enhancement potential improvement and removed invalid not related or critical labels Jun 3, 2019
@thednp thednp closed this as completed in b0b574f Jun 3, 2019
thednp added a commit that referenced this issue Jun 3, 2019
* attempting to fix #233
* added passive event options to resize, touch, scroll events #283 & #280
* code cleanup
@thednp
Copy link
Owner

thednp commented Jun 3, 2019

@felschr @fleck please test latest master and confirm issue is fixed. Need your input before pushing to npm.

Thanks

@jcorporation
Copy link
Contributor

contentString = contentString.trim(); breaks, if the content option is not set. I use only the template option.

@thednp
Copy link
Owner

thednp commented Jun 20, 2019

@jcorporation what do you suggest?

@jcorporation
Copy link
Contributor

trim only, if contentString != null

thednp added a commit that referenced this issue Jul 12, 2019
@jcorporation please test latest master. Thanks
thednp added a commit that referenced this issue Jul 12, 2019
@jcorporation please test latest master. Thanks
@jcorporation
Copy link
Contributor

@thednp: works for me, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement potential improvement
Projects
None yet
Development

No branches or pull requests

4 participants