You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ggam (via GitHub)" <gi...@apache.org> on 2023/10/22 15:49:08 UTC
[PR] style(dataset) Better use of screen space in Edit Dataset modal [superset]
ggam opened a new pull request, #25734:
URL: https://github.com/apache/superset/pull/25734
### SUMMARY
Fixes https://github.com/apache/superset/issues/25675 enabling word wrap on text editor and unfixing modal height (previously 350px).
Note that this enabled word wrap everywhere. I could change it to only apply to the Edit Dataset modal, but I think it makes sense to have this consistently enabled so we can see the most of the SQL expressions, even if it's not manually formatted (adding newlines).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before, see SQL Expression:
After, with word wrap enabled:
Before, Table height fixed to 350px, only 6 rows are shown on my 1920x1080 screen:
After unfixing the heigth:
Smaller screens are not affected as I only change the fixed `height` to `min-height`.
### TESTING INSTRUCTIONS
Edit any dataset, can be from the examples.
### 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/25675
- [ ] 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
Re: [PR] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1826273721
Ping @michael-s-molina @kgabryje @kasiazjc would you please review this PR?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1834146727
> Question about the height - will it always fill the screen or depends on the content? I am a designer, so not sure how some of the properties do work
@kasiazjc it will depend on the content. As you can see on the screenshots of the main comment, the modal maintains the current explicit height on the Metrics tab. It only expands on the Columns tab, which is larger.
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25734:
URL: https://github.com/apache/superset/pull/25734#discussion_r1411057854
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -67,6 +68,7 @@ const defaultProps = {
readOnly: false,
resize: null,
textAreaStyles: {},
+ wrapEnabled: true,
Review Comment:
If you go to Alerts & Reports and try to edit or create new alert, you'll see the component being used.
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25734:
URL: https://github.com/apache/superset/pull/25734#discussion_r1407726995
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -67,6 +68,7 @@ const defaultProps = {
readOnly: false,
resize: null,
textAreaStyles: {},
+ wrapEnabled: true,
Review Comment:
@ggam Can you check how this impacts the `AlertReportModal` that uses this component but limits the number of lines to 15? Should we remove the limit there? Pinging @eschutho for awareness.
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1990090139
@ggam did you have any luck in keeping the modal from changing height here?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1793690278
Friendly ping. Can any of the maintainers review this patch please?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1833953865
> Ping @michael-s-molina @kgabryje @kasiazjc would you please review this PR?
Hi and sorry for the long wait! I think the changes make sense and I mostly agree with them.
Question about the height - will it always fill the screen or depends on the content? I am a designer, so not sure how some of the properties do work
Also thanks @michael-s-molina for tagging me and keeping tabs on the changes 🙏
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on code in PR #25734:
URL: https://github.com/apache/superset/pull/25734#discussion_r1410951866
##########
superset-frontend/src/explore/components/controls/TextAreaControl.jsx:
##########
@@ -67,6 +68,7 @@ const defaultProps = {
readOnly: false,
resize: null,
textAreaStyles: {},
+ wrapEnabled: true,
Review Comment:
@michael-s-molina sure, can you please guide me on what screen is that component shown?
--
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] style(dataset) Better use of screen space in Edit Dataset modal [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1775607175
Thanks @ggam for the pull request.
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1834175720
> > Question about the height - will it always fill the screen or depends on the content? I am a designer, so not sure how some of the properties do work
>
> @kasiazjc it will depend on the content. As you can see on the screenshots of the main comment, the modal maintains the current explicit height on the Metrics tab. It only expands on the Columns tab, which is larger.
Thanks! I think it should have the same, fixed height on all of the columns - changing the height of the modal in general is a bad UX pattern (for example action buttons/footer change place etc which can be confusing). I would suggest setting the height of the modal to for example 80% screen height for all of the tabs to maintain consistent behavior between the tabs
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1814752661
Is there anything I can do to move this forward?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1795414921
@michael-s-molina would you mind reviewing the frontend logic?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1775720191
Thanks @john-bodley I just fixed the linting errors. Could you please rerun the workflows?
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1795429702
> @michael-s-molina would you mind reviewing the frontend logic?
Before reviewing the code, I think we need @kasiazjc's review first. I added her to the reviewers list.
--
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] style(dataset): Better use of screen space in Edit Dataset modal [superset]
Posted by "ggam (via GitHub)" <gi...@apache.org>.
ggam commented on PR #25734:
URL: https://github.com/apache/superset/pull/25734#issuecomment-1990929763
> @ggam did you have any luck in keeping the modal from changing height here?
Sadly I don't have enough styling knowledge to dig into that. If someone gives me some indications, I'd me happy to give it a try
--
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