You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "nytai (via GitHub)" <gi...@apache.org> on 2023/12/05 22:40:31 UTC
[PR] fix(dashboard): title formatting [superset]
nytai opened a new pull request, #26189:
URL: https://github.com/apache/superset/pull/26189
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26189:
URL: https://github.com/apache/superset/pull/26189#discussion_r1418025453
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -113,10 +113,12 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
- /\s/g,
- ' ',
- );
+ sizerRef.current.innerHTML = (currentTitle || placeholder)
Review Comment:
Sorry I meant escape rather than sanitize. It [seems](https://github.com/search?q=repo%3Aapache%2Fsuperset%20escape-html&type=code) like we're already installing the `escape-html` library and thus maybe we should just use that directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #26189:
URL: https://github.com/apache/superset/pull/26189#issuecomment-1841752357
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/26189?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`d2cce51`)](https://app.codecov.io/gh/apache/superset/commit/d2cce51ddf5b9d68bfc1de0c876f5d0b9a2db64c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 69.15% compared to head [(`6394d27`)](https://app.codecov.io/gh/apache/superset/pull/26189?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 58.93%.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #26189 +/- ##
===========================================
- Coverage 69.15% 58.93% -10.22%
===========================================
Files 1944 1944
Lines 75925 75924 -1
Branches 8451 8450 -1
===========================================
- Hits 52505 44745 -7760
- Misses 21235 28994 +7759
Partials 2185 2185
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.64% <ø> (ø)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `61.55% <ø> (-21.26%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/26189/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.81% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/26189?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai commented on code in PR #26189:
URL: https://github.com/apache/superset/pull/26189#discussion_r1418122697
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -113,10 +113,12 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
- /\s/g,
- ' ',
- );
+ sizerRef.current.innerHTML = (currentTitle || placeholder)
Review Comment:
That's the websocket sidecar app (for async queries), but sure we can use that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #26189:
URL: https://github.com/apache/superset/pull/26189#discussion_r1419266715
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -113,10 +114,7 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
- /\s/g,
- ' ',
- );
+ sizerRef.current.innerHTML = escapeHTML(currentTitle || placeholder);
Review Comment:
This is a good improvement, yet it's a breaking change, minimal we need a line on `UPDATING.md`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai merged PR #26189:
URL: https://github.com/apache/superset/pull/26189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai closed pull request #26189: fix(dashboard): title formatting
URL: https://github.com/apache/superset/pull/26189
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #26189:
URL: https://github.com/apache/superset/pull/26189#discussion_r1417774650
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -113,10 +113,12 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
- /\s/g,
- ' ',
- );
+ sizerRef.current.innerHTML = (currentTitle || placeholder)
Review Comment:
@nytai are there libraries we can leverage for sanitizing the HTML?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): title formatting [superset]
Posted by "nytai (via GitHub)" <gi...@apache.org>.
nytai commented on code in PR #26189:
URL: https://github.com/apache/superset/pull/26189#discussion_r1417909415
##########
superset-frontend/src/components/DynamicEditableTitle/index.tsx:
##########
@@ -113,10 +113,12 @@ export const DynamicEditableTitle = ({
// then we can measure the width of that span to resize the input element
useLayoutEffect(() => {
if (sizerRef?.current) {
- sizerRef.current.innerHTML = (currentTitle || placeholder).replace(
- /\s/g,
- ' ',
- );
+ sizerRef.current.innerHTML = (currentTitle || placeholder)
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org