You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2023/06/28 23:48:40 UTC

[GitHub] [superset] betodealmeida opened a new pull request, #24547: feat: customize screenshot width for alerts/reports

betodealmeida opened a new pull request, #24547:
URL: https://github.com/apache/superset/pull/24547

   <!---
   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 -->
   
   This PR is part 1 of 2. It adds a new input to the Alerts & Reports create/edit modals, as well as the report modal from charts and dashboards. In the new input users can configure a custom width for the screenshots
   
   This PR updates the model, adds the migration to introduce a `custom_width` column (and `custom_height`, though it's not used), and add the affordances to set the custom width in the create/edit flows.
   
   The next PR will read the information from the DB and use it when running the alerts and reports.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   New:
   
   ![Screenshot 2023-06-28 at 16-45-58 FCC New Coder Survey 2018](https://github.com/apache/superset/assets/1534870/2693fe7e-f7b9-4983-ba6b-05470bee1095)
   
   ![Screenshot 2023-06-28 at 16-47-09 FCC New Coder Survey 2018](https://github.com/apache/superset/assets/1534870/041273a2-c4e0-4579-b841-5f6fc6e9dd8e)
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   
   Create a few alerts & reports with custom width, then check the DB:
   
   ```
   customize_screenshot_width=# SELECT id, custom_width FROM report_schedule;
    id | custom_width
   ----+--------------
     2 |          760
     3 |          600
     1 |          750
   (3 rows)
   ```
   
   ### 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

[GitHub] [superset] github-actions[bot] commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612252445

   @yousoph Container image not yet published for this PR. Please try again when build is complete.


-- 
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] hughhhh commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246840503


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -853,12 +864,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     }).then(response => setChartVizType(response.json.result.viz_type));
 
   // Handle input/textarea updates
-  const onTextChange = (
+  const onInputChange = (
     event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
   ) => {
     const { target } = event;
+    const value =
+      target.type === 'number'
+        ? parseInt(target.value, 10) || null
+        : target.value;
 
-    updateAlertState(target.name, target.value);
+    updateAlertState(target.name, value);

Review Comment:
   deconstruct `name` as well



-- 
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 #24547: feat: customize screenshot width for alerts/reports

Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612247881

   ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   > Merging [#24547](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5c5f567) into [master](https://app.codecov.io/gh/apache/superset/commit/750113441cd0160ed53e918253a7256466028ac0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7501134) will **increase** coverage by `0.14%`.
   > The diff coverage is `72.72%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #24547      +/-   ##
   ==========================================
   + Coverage   68.93%   69.07%   +0.14%     
   ==========================================
     Files        1903     1903              
     Lines       74027    74047      +20     
     Branches     8118     8117       -1     
   ==========================================
   + Hits        51027    51149     +122     
   + Misses      20889    20787     -102     
     Partials     2111     2111              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `53.90% <60.60%> (?)` | |
   | mysql | `79.40% <72.72%> (-0.01%)` | :arrow_down: |
   | postgres | `79.48% <72.72%> (-0.02%)` | :arrow_down: |
   | presto | `53.79% <60.60%> (?)` | |
   | python | `83.47% <72.72%> (+0.29%)` | :arrow_up: |
   | sqlite | `77.99% <72.72%> (-0.01%)` | :arrow_down: |
   | unit | `54.65% <60.60%> (+<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.
   
   | [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...rset-frontend/src/components/ReportModal/index.tsx](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUmVwb3J0TW9kYWwvaW5kZXgudHN4) | `79.03% <ø> (ø)` | |
   | [...set-frontend/src/components/ReportModal/styles.tsx](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvUmVwb3J0TW9kYWwvc3R5bGVzLnRzeA==) | `100.00% <ø> (ø)` | |
   | [...-frontend/src/features/alerts/AlertReportModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2ZlYXR1cmVzL2FsZXJ0cy9BbGVydFJlcG9ydE1vZGFsLnRzeA==) | `54.22% <ø> (-0.23%)` | :arrow_down: |
   | [superset/reports/api.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9hcGkucHk=) | `90.29% <ø> (ø)` | |
   | [superset/charts/api.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | `87.78% <50.00%> (ø)` | |
   | [superset/reports/schemas.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9zY2hlbWFzLnB5) | `91.08% <50.00%> (-7.75%)` | :arrow_down: |
   | [superset/config.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `92.02% <100.00%> (+0.04%)` | :arrow_up: |
   | [superset/reports/models.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVwb3J0cy9tb2RlbHMucHk=) | `100.00% <100.00%> (ø)` | |
   | [superset/utils/screenshots.py](https://app.codecov.io/gh/apache/superset/pull/24547?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvc2NyZWVuc2hvdHMucHk=) | `51.63% <100.00%> (+2.06%)` | :arrow_up: |
   
   ... and [8 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24547/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   :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] betodealmeida commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1247034331


##########
superset/reports/schemas.py:
##########
@@ -208,10 +209,34 @@ class ReportSchedulePostSchema(Schema):
         dump_default=None,
     )
     force_screenshot = fields.Boolean(dump_default=False)
+    custom_width = fields.Integer(
+        metadata={
+            "description": _("Custom width of the screenshot in pixels"),
+            "example": 1000,
+        },
+        allow_none=True,
+        required=False,
+        default=None,
+    )
+
+    @validates("custom_width")
+    def validate_custom_width(self, value: int) -> None:  # pylint: disable=no-self-use

Review Comment:
   Good point, we currently show validation errors post-`POST`, but it would be great to show client-side.
   
   Since this is time sensitive  I'm going to do that in a separate PR, though, are you OK with 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

[GitHub] [superset] kgabryje commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246825788


##########
superset-frontend/src/components/ReportModal/index.tsx:
##########
@@ -257,6 +263,29 @@ function ReportModal({
       </div>
     </>
   );
+  const renderCustomWidthSection = (
+    <StyledInputContainer>
+      <div
+        className="control-label"
+        css={(theme: SupersetTheme) => CustomWidthHeaderStyle(theme)}
+      >
+        {TRANSLATIONS.CUSTOM_SCREENSHOT_WIDTH_TEXT}
+      </div>
+      <div className="input-container">
+        <input

Review Comment:
   Can we use Antd input?



##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1471,6 +1486,27 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
                 </div>
               </>
             )}
+            {isScreenshot && (
+              <StyledInputContainer>
+                <div
+                  className="control-label"
+                  css={(theme: SupersetTheme) => CustomWidthHeaderStyle(theme)}

Review Comment:
   same as above



##########
superset-frontend/src/components/ReportModal/index.tsx:
##########
@@ -257,6 +263,29 @@ function ReportModal({
       </div>
     </>
   );
+  const renderCustomWidthSection = (
+    <StyledInputContainer>
+      <div
+        className="control-label"
+        css={(theme: SupersetTheme) => CustomWidthHeaderStyle(theme)}

Review Comment:
   nit: `css={CustomWidthHeaderStyle}`



##########
superset/reports/schemas.py:
##########
@@ -208,10 +209,34 @@ class ReportSchedulePostSchema(Schema):
         dump_default=None,
     )
     force_screenshot = fields.Boolean(dump_default=False)
+    custom_width = fields.Integer(
+        metadata={
+            "description": _("Custom width of the screenshot in pixels"),
+            "example": 1000,
+        },
+        allow_none=True,
+        required=False,
+        default=None,
+    )
+
+    @validates("custom_width")
+    def validate_custom_width(self, value: int) -> None:  # pylint: disable=no-self-use

Review Comment:
   can we add the same validation to the UI? With some danger text under the input or sth



##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -1471,6 +1486,27 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
                 </div>
               </>
             )}
+            {isScreenshot && (
+              <StyledInputContainer>
+                <div
+                  className="control-label"
+                  css={(theme: SupersetTheme) => CustomWidthHeaderStyle(theme)}
+                >
+                  {TRANSLATIONS.CUSTOM_SCREENSHOT_WIDTH_TEXT}
+                </div>
+                <div className="input-container">
+                  <input

Review Comment:
   Can we use Antd here too?



-- 
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] betodealmeida commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246850389


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -853,12 +864,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     }).then(response => setChartVizType(response.json.result.viz_type));
 
   // Handle input/textarea updates
-  const onTextChange = (
+  const onInputChange = (
     event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
   ) => {
     const { target } = event;
+    const value =
+      target.type === 'number'
+        ? parseInt(target.value, 10) || null
+        : target.value;
 
-    updateAlertState(target.name, target.value);
+    updateAlertState(target.name, value);

Review Comment:
   Ah, you mean on the line above?
   
   ```js
   const { target, target : { name } } = event;
   ```
   
   ?



-- 
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] betodealmeida commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246840069


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -853,12 +864,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     }).then(response => setChartVizType(response.json.result.viz_type));
 
   // Handle input/textarea updates
-  const onTextChange = (
+  const onInputChange = (
     event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
   ) => {
     const { target } = event;
+    const value =
+      target.type === 'number'

Review Comment:
   No, it's just the `<input type="XXX" />` value.



##########
superset-frontend/src/components/ReportModal/index.tsx:
##########
@@ -257,6 +263,29 @@ function ReportModal({
       </div>
     </>
   );
+  const renderCustomWidthSection = (
+    <StyledInputContainer>
+      <div
+        className="control-label"
+        css={(theme: SupersetTheme) => CustomWidthHeaderStyle(theme)}

Review Comment:
   D'oh, will fix. Thanks!



-- 
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] hughhhh commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1247104154


##########
superset/reports/schemas.py:
##########
@@ -208,10 +209,34 @@ class ReportSchedulePostSchema(Schema):
         dump_default=None,
     )
     force_screenshot = fields.Boolean(dump_default=False)
+    custom_width = fields.Integer(

Review Comment:
   what about `custom_height`



-- 
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] github-actions[bot] commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612252455

   @yousoph Ephemeral environment creation failed. Please check the Actions logs for details.


-- 
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] yousoph commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612252059

   /testenv up FEATURE_ALERT_REPORTS = True 


-- 
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] github-actions[bot] commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1613835877

   Ephemeral environment shutdown and build artifacts deleted.


-- 
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] betodealmeida merged pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida merged PR #24547:
URL: https://github.com/apache/superset/pull/24547


-- 
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] github-actions[bot] commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612400535

   @yousoph Ephemeral environment spinning up at http://54.218.34.104:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.


-- 
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] hughhhh commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246821979


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -853,12 +864,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     }).then(response => setChartVizType(response.json.result.viz_type));
 
   // Handle input/textarea updates
-  const onTextChange = (
+  const onInputChange = (
     event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
   ) => {
     const { target } = event;
+    const value =
+      target.type === 'number'

Review Comment:
   is there an enum we can attach to this?



-- 
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] betodealmeida commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1246843637


##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -853,12 +864,16 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
     }).then(response => setChartVizType(response.json.result.viz_type));
 
   // Handle input/textarea updates
-  const onTextChange = (
+  const onInputChange = (
     event: React.ChangeEvent<HTMLTextAreaElement | HTMLInputElement>,
   ) => {
     const { target } = event;
+    const value =
+      target.type === 'number'
+        ? parseInt(target.value, 10) || null
+        : target.value;
 
-    updateAlertState(target.name, target.value);
+    updateAlertState(target.name, value);

Review Comment:
   But `target.name` is not used anywhere else, I'd have to replace:
   
   ```
   updateAlertState(target.name, value);
   ```
   
   With
   
   ```
   const { name } = target;
   updateAlertState(name, value);
   ```



-- 
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] betodealmeida commented on a diff in pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on code in PR #24547:
URL: https://github.com/apache/superset/pull/24547#discussion_r1247109400


##########
superset/reports/schemas.py:
##########
@@ -208,10 +209,34 @@ class ReportSchedulePostSchema(Schema):
         dump_default=None,
     )
     force_screenshot = fields.Boolean(dump_default=False)
+    custom_width = fields.Integer(

Review Comment:
   Custom height is not implemented, I added it to the model so we don't need to do another migration in the future.



-- 
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] yousoph commented on pull request #24547: feat: customize screenshot width for alerts/reports

Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #24547:
URL: https://github.com/apache/superset/pull/24547#issuecomment-1612398807

   /testenv up FEATURE_ALERT_REPORTS = True


-- 
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