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

Datahub: Fix abstract display in record preview #672

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

tkohr
Copy link
Collaborator

@tkohr tkohr commented Nov 3, 2023

Remove unnecessary whitespaces from html which can break line-clamp behaviour. Example abstract. Has only been reproduced on firefox.

line-clamp

@tkohr tkohr added the bug Something isn't working label Nov 3, 2023
Copy link
Contributor

github-actions bot commented Nov 3, 2023

Affected libs: ui-search, feature-search, feature-router, feature-map, feature-dataviz, feature-record, util-shared, ui-elements, feature-catalog, ui-catalog, ui-widgets, feature-editor, ui-inputs,
Affected apps: metadata-editor, datahub, demo, webcomponents, search, map-viewer, metadata-converter, datafeeder,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@coveralls
Copy link

coveralls commented Nov 3, 2023

Coverage Status

coverage: 82.269% (-4.3%) from 86.617%
when pulling 8875d8e on sanitize-html-whitespaces
into dbc10d3 on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is taking the stripHtml function too far. Keeping whitespace and at the very least line breaks can be considered a feature, although I'm not sure if it would break anything.

Maybe adding another util removeWhitespace would make sense? And then the two can be combined for the results abstract.

@tkohr tkohr force-pushed the sanitize-html-whitespaces branch from ba55fbf to 8875d8e Compare November 6, 2023 09:32
@tkohr
Copy link
Collaborator Author

tkohr commented Nov 6, 2023

Good point, I completely agree (although the stripHtml function is currently only used for the preview abstract). This should make things much more clear. I've adapted accordingly.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @tkohr, even though it works exactly like before I believe you still improved the readability for future contributors ;)

@tkohr tkohr merged commit 499c9ac into main Nov 6, 2023
7 checks passed
@tkohr tkohr deleted the sanitize-html-whitespaces branch November 6, 2023 10:20
jahow pushed a commit that referenced this pull request Nov 13, 2023
since it can break line-clamp behaviour

Backport from  #672
@jahow
Copy link
Collaborator

jahow commented Nov 13, 2023

backported in a230b2c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.0.x bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants