You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2023/01/10 10:27:19 UTC
[GitHub] [superset] artemonsh opened a new pull request, #22660: fix(dashboard): remove horizontal scrolling & allow showing dashboard title in 2 lines
artemonsh opened a new pull request, #22660:
URL: https://github.com/apache/superset/pull/22660
<!---
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 -->
1. For dashboards with long title all elements don't fit to screen anymore and horizontal scrolling appears. This PR is an attempt to solve this issue.
2. I suggest to show not 1 but 2 lines of the title if it is long enough (which is a common practice in my company). The overflow will be hidden as it was before. It is done with help of CSS properties webkit-line-clamp and webkit-box-orient (example [here](https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-line-clamp)).
### BEFORE
### AFTER
### BEFORE vs AFTER video (with sound)
https://user-images.githubusercontent.com/53895552/211523820-fe913c9f-d387-450c-ada0-72a1ba7572ee.mp4
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Create a dashboard with a very long title. You can take it from this website: https://www.lipsum.com
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: https://github.com/apache/superset/issues/22520
- [ ] Required feature flags:
- [x] 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
[GitHub] [superset] artemonsh commented on pull request #22660: fix(dashboard): remove horizontal scrolling for long-titled dashboards & allow showing dashboard title in 2 lines
Posted by GitBox <gi...@apache.org>.
artemonsh commented on PR #22660:
URL: https://github.com/apache/superset/pull/22660#issuecomment-1385566687
@villebro could you please take a look?
--
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
[GitHub] [superset] codecov[bot] commented on pull request #22660: fix(dashboard): remove horizontal scrolling & allow showing dashboard title in 2 lines
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22660:
URL: https://github.com/apache/superset/pull/22660#issuecomment-1377099129
# [Codecov](https://codecov.io/gh/apache/superset/pull/22660?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22660](https://codecov.io/gh/apache/superset/pull/22660?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2c48f05) into [master](https://codecov.io/gh/apache/superset/commit/159dcd7e62e9466e2da4ad81cd25c06770fb4a5e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (159dcd7) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head 2c48f05 differs from pull request most recent head d4445f7. Consider uploading reports for the commit d4445f7 to get more accurate results
```diff
@@ Coverage Diff @@
## master #22660 +/- ##
=======================================
Coverage 67.14% 67.14%
=======================================
Files 1869 1869
Lines 71523 71523
Branches 7814 7814
=======================================
Hits 48022 48022
Misses 21460 21460
Partials 2041 2041
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `53.98% <ø> (ø)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22660?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...tend/src/components/DynamicEditableTitle/index.tsx](https://codecov.io/gh/apache/superset/pull/22660?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvRHluYW1pY0VkaXRhYmxlVGl0bGUvaW5kZXgudHN4) | `74.07% <ø> (ø)` | |
| [...d/components/DashboardBuilder/DashboardBuilder.tsx](https://codecov.io/gh/apache/superset/pull/22660?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZEJ1aWxkZXIvRGFzaGJvYXJkQnVpbGRlci50c3g=) | `73.58% <ø> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] artemonsh commented on a diff in pull request #22660: fix(dashboard): remove horizontal scrolling for long-titled dashboards & allow showing dashboard title in 2 lines
Posted by GitBox <gi...@apache.org>.
artemonsh commented on code in PR #22660:
URL: https://github.com/apache/superset/pull/22660#discussion_r1069393079
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -83,7 +83,7 @@ type DashboardBuilderProps = {};
const StyledDiv = styled.div`
display: grid;
- grid-template-columns: auto 1fr;
+ grid-template-columns: auto minmax(0, 1fr);
Review Comment:
This is the only change to fix horizontal scrolling
--
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): remove horizontal scrolling for long-titled dashboards & allow showing dashboard title in 2 lines [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #22660:
URL: https://github.com/apache/superset/pull/22660#issuecomment-1930654659
Thanks for the fantastic PR description/videos (narration is such an amazing and unusual thing around here!). Also, sorry this slid under our collective radar for so long. Are you able to give this a quick rebase to bring it up to a mergeable state?
Also, I haven't checked the caniuse.com entries for these css properties yet, but I'm assuming these are pretty friendly for most browsers by now and might not need the webkit vendor prefix anymore if we're lucky :D
--
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