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

strip_html should leave space between block elements #54

Closed
dfdan opened this issue Feb 11, 2022 · 2 comments
Closed

strip_html should leave space between block elements #54

dfdan opened this issue Feb 11, 2022 · 2 comments
Labels
bug Something isn't working wontfix This will not be worked on

Comments

@dfdan
Copy link
Member

dfdan commented Feb 11, 2022

Package version (if known): all

Describe the bug

When strip_html is passed string containing multiple paragaphs (eg by rdm-records' ui serialiser), entries with multiple paragraphs are concatenated without spacing. This looks poor and reduces readability.

Steps to Reproduce

See screenshots below (from Invenio RDM), or:

In [3]: strip_html('<p>a</p><p>b</p>')
Out[3]: 'ab'

Expected behaviour

A single space should be inserted between the paragraphs content to ensure the stripped description is readable, ie:

In [3]: strip_html('<p>a</p><p>b</p>')
Out[3]: 'a b'

Screenshots (if applicable)

Invenio RDM record description with 2 paragraphs:
Screenshot 2022-02-11 at 18 17 06

Renders in search without space between paragraphs:
Screenshot 2022-02-11 at 21 41 07

Additional context

I have made a local workaround that inserts a space after any paragraph closing tag before calling strip_html. Perhaps
a proper fix would use html.parse to pass the description to strip_html 1 paragraph at a time? Happy to give that a go if it seems sensible.

@dfdan dfdan added the bug Something isn't working label Feb 11, 2022
@dfdan
Copy link
Member Author

dfdan commented Feb 16, 2022

I've looked into this further - there is a known issue with bleach addressed by this PR: mozilla/bleach#642

I've confirmed this fixes the issue, so we can just bump dependencies here if/when that PR merges.

@lnielsen lnielsen added the wontfix This will not be worked on label Feb 16, 2022
@lnielsen
Copy link
Member

Ok, then I'm closing the ticket since we'll get it in automatically with the latest bleach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants