You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/07/25 14:56:03 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #24801: fix: Filters alert width
michael-s-molina opened a new pull request, #24801:
URL: https://github.com/apache/superset/pull/24801
### SUMMARY
Fixes the filters alert width and also changes the modal width.
Following @kasiazjc's [comment](https://github.com/apache/superset/pull/24559#issuecomment-1632099115) in https://github.com/apache/superset/pull/24559:
> I think in general we have to bump the width of the modal in the default mode to ~880px (and resize just after it not longer fits in the screen), so that everything is more visible.
I adjusted the modal to have a fixed base width instead of percent based one. I set it to 1000px instead of 880px as a middle ground.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<img width="1781" alt="Screenshot 2023-07-25 at 11 36 58" src="https://github.com/apache/superset/assets/70410625/015eb444-bd9d-42f0-8a03-f6a83f9f7c4e">
<img width="1770" alt="Screenshot 2023-07-25 at 11 46 35" src="https://github.com/apache/superset/assets/70410625/161d359c-63a1-4b47-93bc-a655c5d90934">
### TESTING INSTRUCTIONS
Make sure the the width of the filters' alert takes the available space.
### ADDITIONAL INFORMATION
- [ ] 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
[GitHub] [superset] michael-s-molina commented on pull request #24801: fix: Filters alert width
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #24801:
URL: https://github.com/apache/superset/pull/24801#issuecomment-1652254186
> Can be convinced that 1000px is better though if you give me reasons @michael-s-molina :D
I was worried it wouldn't work for Airbnb but I checked with @justinpark and 880px is fine for most cases. I changed the width back to 880px.
--
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 #24801: fix: Filters alert width
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24801:
URL: https://github.com/apache/superset/pull/24801#issuecomment-1650135434
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24801?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24801](https://app.codecov.io/gh/apache/superset/pull/24801?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (af1a018) into [master](https://app.codecov.io/gh/apache/superset/commit/4a81284056b42faf5be29d9e020a3556a66361bb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (4a81284) will **increase** coverage by `0.00%`.
> The diff coverage is `50.00%`.
> :exclamation: Current head af1a018 differs from pull request most recent head cdc4953. Consider uploading reports for the commit cdc4953 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24801 +/- ##
=======================================
Coverage 68.95% 68.96%
=======================================
Files 1902 1902
Lines 73998 73999 +1
Branches 8195 8195
=======================================
+ Hits 51029 51030 +1
Misses 20850 20850
Partials 2119 2119
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `55.79% <50.00%> (+<0.01%)` | :arrow_up: |
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.
| [Files Changed](https://app.codecov.io/gh/apache/superset/pull/24801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...tersConfigModal/Footer/CancelConfirmationAlert.tsx](https://app.codecov.io/gh/apache/superset/pull/24801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0Zvb3Rlci9DYW5jZWxDb25maXJtYXRpb25BbGVydC50c3g=) | `100.00% <ø> (ø)` | |
| [...eFilters/FiltersConfigModal/FiltersConfigModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24801?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL25hdGl2ZUZpbHRlcnMvRmlsdGVyc0NvbmZpZ01vZGFsL0ZpbHRlcnNDb25maWdNb2RhbC50c3g=) | `71.09% <50.00%> (+0.13%)` | :arrow_up: |
: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=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
[GitHub] [superset] michael-s-molina merged pull request #24801: fix: Filters alert width
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #24801:
URL: https://github.com/apache/superset/pull/24801
--
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] kasiazjc commented on pull request #24801: fix: Filters alert width
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24801:
URL: https://github.com/apache/superset/pull/24801#issuecomment-1652224850
> ### SUMMARY
> Fixes the filters alert width and also changes the modal width.
>
> Following @kasiazjc's [comment](https://github.com/apache/superset/pull/24559#issuecomment-1632099115) in #24559:
>
> > I think in general we have to bump the width of the modal in the default mode to ~880px (and resize just after it not longer fits in the screen), so that everything is more visible.
>
> I adjusted the modal to have a fixed base width instead of percent based one. I set it to 1000px instead of 880px as a middle ground.
>
> ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
> <img alt="Screenshot 2023-07-25 at 11 36 58" width="1781" src="https://user-images.githubusercontent.com/70410625/255940080-015eb444-bd9d-42f0-8a03-f6a83f9f7c4e.png"> <img alt="Screenshot 2023-07-25 at 11 46 35" width="1770" src="https://user-images.githubusercontent.com/70410625/255940111-161d359c-63a1-4b47-93bc-a655c5d90934.png">
> ### TESTING INSTRUCTIONS
> Make sure the the width of the filters' alert takes the available space.
>
> ### ADDITIONAL INFORMATION
> * [ ] 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
I think on the smaller screens, modals that are that big of a size can be overwhelming (that's why I would prefer to avoid them) and I do not think that we add that much value with the increased size (especially because with 880x the fields are already bigger than our default + 240px seems like enough). I think we use width of 880px we use in the dashboard properties etc and the 1000px+ is used for alerts&reports as there is a lot of content and different inputs etc.
Can be convinced that 1000px is better though if you give me reasons @michael-s-molina :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
[GitHub] [superset] kasiazjc commented on pull request #24801: fix: Filters alert width
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24801:
URL: https://github.com/apache/superset/pull/24801#issuecomment-1652259160
> > Can be convinced that 1000px is better though if you give me reasons @michael-s-molina :D
>
> I was worried it wouldn't work for Airbnb but I checked with @justinpark and 880px is fine for most cases. I changed the width back to 880px.
Thank you @michael-s-molina ❤️
--
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