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

Handlebar multiple problems #22548

Closed
majkinetor opened this issue Dec 29, 2022 · 13 comments
Closed

Handlebar multiple problems #22548

majkinetor opened this issue Dec 29, 2022 · 13 comments

Comments

@majkinetor
Copy link

majkinetor commented Dec 29, 2022

I am reporting here multiple problems on Handlebars with latest master (pulled on 2022-12-19 at 12h). This is unfortunate since its IMO the most flexible chart type out there that allows arbitrary detailed customizations on KPI like data and small lists.

Using dataset:

select 1.1 as a, 1.2 as b
union all
select 2.1 as a, 2.2 as b
Screenshot

image

1. Raw records

This happens with any dataset and it doesn't happen if there is no dataset. Error is n.map is not a function:

Screenshot

image

For example, I used trivial dataset `select 1 as a, 2 as b`. As soon as you show this tab there is this error.

2. Css visible in the otput

As soon as you add something in CSS it becomes visible in the output (here just space). Reported at #22410.

Screenshot

image

With sanitation on, it kinda works but also problematic. Here I just add new rule and it goes to wack:
Screenshot

image

If I move rule to the top, then it works, but last now doesn't. Seems like markdown problem. Why would you process CSS with markdown ?

3. Santiation

HTML is sanitized, althogh there is no icon showing that like on CSS. Noticed how class is gone everywhere:

Template
<table class='data-table'>
  <thead>
    <tr>
      <th>Column 1</th>
      <th></th>
      <th>Column 2</th>
    </tr>
  </thead>
  <tbody>
  {{#each data}}
  <tr class="r{{@index}}">
    <td class="c1">{{this.a}}</td>
    <td>&nbsp;&nbsp;</td>
    <td class="c2">{{this.b}}</td>
  </tr>
  {{/each}}
  </tbody>
</table>
Render

image

4. Weird behavior with new line

If I just add a new line in the table:

Screenshot

image

This seems unexpected, probably markdown artifact ?

5. View as table / dataset

View as table simply doesn't work, it also draws handlebar template:

screenshot

image

Viewing and setting dataset columns and metrics is almost impossible:

screenshot

image

image


Perhaps just better documentation should be done but nevertheless, it feels like half-done-work with this chart, too many gotchas all around, while certainly being the only way to craeate arbitrary UI and should be well polished (and promoted, both in docs and examples). Also, Template and CSS editors should be easier to work with (bigger/maximizable etc.)

@majkinetor majkinetor added the #bug Bug report label Dec 29, 2022
@majkinetor majkinetor changed the title Handlebars raw records error Handlebars raw records error: n.map is not a function Dec 29, 2022
@majkinetor majkinetor changed the title Handlebars raw records error: n.map is not a function Handlebar multiple errors Dec 29, 2022
@majkinetor
Copy link
Author

majkinetor commented Dec 29, 2022

CSS and sanition work correctly when HTML_SANITIZATION = False.

@sfirke
Copy link
Member

sfirke commented Dec 29, 2022

I'm also experiencing problems with handlebars in 2.0.1. Can you share what you are supplying to HTML_SANITIZATION_SCHEMA_EXTENSIONS in order to get that red and yellow CSS to render in your #2 example above?

@majkinetor
Copy link
Author

majkinetor commented Dec 29, 2022

@sfirke, I just set it to false for now:

# Sanitizes the HTML content used in markdowns to allow its rendering in a safe manner.
# Disabling this option is not recommended for security reasons. If you wish to allow
# valid safe elements that are not included in the default sanitization schema, use the
# HTML_SANITIZATION_SCHEMA_EXTENSIONS configuration.
HTML_SANITIZATION = False

There is no HTML_SANITIZATION_SCHEMA_EXTENSIONS in my config.

@sfirke
Copy link
Member

sfirke commented Dec 29, 2022

Got it. I am seeking an example of how, with Sanitization = True, I could allow something simple like font-size to render. As far as I can tell, there is no documentation for Handlebars chart anywhere - I searched the Superset docs and the Preset list of documented charts.

EDIT: This chunk from this Preset blog post got my charts looking correct again, with font-size rendering:

HTML_SANITIZATION_SCHEMA_EXTENSIONS = {
  "attributes": {
    "*": ["style","className"],
  },
  "tagNames": ["style"],
}

@majkinetor
Copy link
Author

majkinetor commented Dec 29, 2022

Yes, thats why I noted:

Perhaps just better documentation should be done but nevertheless, it feels like half-done-work with this chart, to many gotchas all around, while certainly being the only way to craeate arbitrary UI and should be well polished (and promoted, both in docs and examples).

With sanitation off, css works normaly, here is one example, even animations work:

image

NOTE: DO NOT HAVE EMPTY LINES IN CSS.

@majkinetor majkinetor changed the title Handlebar multiple errors Handlebar multiple problems Dec 29, 2022
@rusackas
Copy link
Member

rusackas commented Jan 4, 2023

There are some details about how to configure HTML_SANITIZATION_SCHEMA_EXTENSIONS to allow the class attribute (or other things) in this blog post which I hope helps clarify some of it.

@GetasOK
Copy link

GetasOK commented Jan 11, 2023

With first problem (Raw records) i am select table component, switch to raw records, do that i need and switch back to handlebars. Error still visible, but data is available.

@rusackas
Copy link
Member

Honestly, I feel like we should break this up into separate Issues. Sounds like we've collectively resolved one of them, but the others need to be teased apart and tackled separately, or this issue will be difficult to track and close.

CC @jdbranham and @villebro in case you or anyone you know of has bandwidth to start chipping away at some of these things.

@rusackas
Copy link
Member

We may be able to resolve issues with empty lines in the HTML template and/or the CSS by making Prettier a full dependency (rather than a dev dependency) and using prettier.format to clean up the user-input code. I'm just not sure if this would be equally as annoying as it is helpful.

@majkinetor
Copy link
Author

majkinetor commented Jan 23, 2023

  1. Isn't it better to make markdown an option, disabled by default? Or even removing markdown processing here ?
  2. Why introducing prettier for this, you can just remove empty lines.

But I still think that markdown here is probably not a good idea, I beleive that this is just one way it could mess things up.

@rusackas
Copy link
Member

Is anyone willing to break this up into separate issues? I'd close this as stale, but I know at least some of this is still valid. We should just make individual, more easily actionable, tickets.

@sfirke
Copy link
Member

sfirke commented Feb 15, 2024

@rusackas agreed that some of this is still valid and that these multi-part tickets are not great. Michael has been very good recently pushing me to submit multiple separate tickets that are actionable. Going forward I'll try to issue the request for multiple tickets immediately upon reading.

@rusackas
Copy link
Member

Hey folks... this seems to have gone stale. I'll be using the plugin a bit more in the near future, and will report any issues I find separately. If closing this ruffles any feathers, please feel free to copy/paste/submit any of the individual problems as individual Issues, and hopefully we can start tackling them!

@rusackas rusackas closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants