-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
fix(dashboard): title formatting #26189
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26189 +/- ##
==========================================
+ Coverage 67.00% 69.20% +2.20%
==========================================
Files 1944 1945 +1
Lines 75927 76114 +187
Branches 8451 8521 +70
==========================================
+ Hits 50873 52676 +1803
+ Misses 22869 21234 -1635
- Partials 2185 2204 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/\s/g, | ||
' ', | ||
); | ||
sizerRef.current.innerHTML = (currentTitle || placeholder) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nytai are there libraries we can leverage for sanitizing the HTML?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to sanitize (ie, remove) html or just escape it? I think the purpose of this code is to render the title to the DOM so we can measure how much space it takes up, so we probably want to escape it.
Looking at an escape-html lib, it's pretty basic. Eg https://github.com/component/escape-html/blob/master/index.js
Happy to update the PR to use this lib if we think it's best, but... you know the joke about node modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I meant escape rather than sanitize. It seems like we're already installing the escape-html
library and thus maybe we should just use that directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the websocket sidecar app (for async queries), but sure we can use that.
/\s/g, | ||
' ', | ||
); | ||
sizerRef.current.innerHTML = escapeHTML(currentTitle || placeholder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good improvement, yet it's a breaking change, minimal we need a line on UPDATING.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well now it's nice and simple! :D
(cherry picked from commit 88fb342)
(cherry picked from commit 88fb342)
(cherry picked from commit 88fb342)
(cherry picked from commit 88fb342)
SUMMARY
Titles with special characters are not rendered correctly. This PR ensures that special characters are handled correctly and titles match the user input
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION