fix: Fixed spacing in alert modal #22066

lyndsiWilliams · 2022-11-08T19:32:16Z

SUMMARY

This PR fixes the spacing under the "Message content" section of the alert modal.

Note: When I adjusted the spacing I noticed a few spots in the component that needed translating/cleanup so I added that to this PR as well.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

AFTER (extended)

TESTING INSTRUCTIONS

Go to the Alerts & reports page and click the "+ ALERT" button
Observe the spacing between elements

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

codecov · 2022-11-08T19:33:03Z

Codecov Report

Merging #22066 (5001014) into master (49f1cfc) will increase coverage by 0.01%.
The diff coverage is 76.49%.

❗ Current head 5001014 differs from pull request most recent head f7f6303. Consider uploading reports for the commit f7f6303 to get more accurate results

@@            Coverage Diff             @@
##           master   #22066      +/-   ##
==========================================
+ Coverage   66.87%   66.89%   +0.01%     
==========================================
  Files        1847     1850       +3     
  Lines       70558    70703     +145     
  Branches     7735     7750      +15     
==========================================
+ Hits        47183    47294     +111     
- Misses      21374    21393      +19     
- Partials     2001     2016      +15

Flag	Coverage Δ
javascript	`53.86% <59.75%> (+0.02%)`	⬆️
mysql	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ugins/legacy-plugin-chart-pivot-table/src/index.js	`66.66% <ø> (ø)`
...d/plugins/legacy-plugin-chart-treemap/src/index.js	`66.66% <ø> (ø)`
...plugins/legacy-preset-chart-nvd3/src/Area/index.js	`66.66% <ø> (ø)`
.../plugins/legacy-preset-chart-nvd3/src/Bar/index.js	`66.66% <ø> (ø)`
...plugins/legacy-preset-chart-nvd3/src/Line/index.js	`66.66% <ø> (ø)`
.../plugin-chart-echarts/src/Timeseries/Area/index.ts	`33.33% <ø> (ø)`
...-chart-echarts/src/Timeseries/Regular/Bar/index.ts	`33.33% <0.00%> (ø)`
...chart-echarts/src/Timeseries/Regular/Line/index.ts	`33.33% <ø> (ø)`
.../plugins/plugin-chart-echarts/src/Treemap/index.ts	`50.00% <ø> (ø)`
...ars/src/components/ControlHeader/controlHeader.tsx	`50.00% <ø> (ø)`
... and 57 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lyndsiWilliams · 2022-11-08T21:07:58Z

/testenv up

github-actions · 2022-11-08T21:09:59Z

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

eschutho · 2022-11-08T23:40:59Z

@lyndsiWilliams This looks great!! Can we also left align it to either match up with the dashboard/chart select box or the email select box? Maybe @yousoph or a designer my have more opinions there.

rusackas · 2022-12-05T22:04:44Z

@lyndsiWilliams @yousoph will there be any more actions happening on this PR, or should we merge it as-is?

lyndsiWilliams · 2022-12-06T03:17:50Z

@lyndsiWilliams @yousoph will there be any more actions happening on this PR, or should we merge it as-is?

More actions for sure! @yousoph , @eschutho , Karan, and I all got together on zoom to check this out together and discuss the styling. We decided to, for now, just get everything aligned with the 16px border to clean it up a little more. I just pushed up the changes in this commit.

It now looks like this:

lyndsiWilliams · 2022-12-06T13:29:39Z

/testenv up

github-actions · 2022-12-06T13:34:33Z

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

Antonio-RiveroMartnez · 2022-12-06T17:21:58Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

@@ -1300,7 +1309,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
                    placeholder={t('Time in seconds')}
                    onChange={onTimeoutVerifyChange}
                  />
-                  <span className="input-label">seconds</span>
+                  <span className="input-label">{t('seconds')}</span>


I totally liked what you did here: https://github.com/apache/superset/pull/22262/files#diff-bbf91464669d98b9e120f57ce26155804f7238a277fcc30c0d805a943f6c73edR98 , could we do the same for these?

You got it! Done in this commit. (I think I got them all!)

lyndsiWilliams · 2022-12-07T01:58:55Z

/testenv up ALERT_REPORTS=true

github-actions · 2022-12-07T02:02:45Z

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

lyndsiWilliams · 2022-12-07T02:29:30Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2022-12-07T02:33:45Z

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

lyndsiWilliams · 2022-12-07T02:41:55Z

Closing and reopening to reset the ephemeral environment, things aren't showing up properly 🤔

github-actions · 2022-12-07T02:42:19Z

Ephemeral environment shutdown and build artifacts deleted.

lyndsiWilliams · 2022-12-07T13:33:02Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2022-12-07T13:35:04Z

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

lyndsiWilliams · 2022-12-07T17:40:28Z

The ephemeral environment doesn't seem to be working correctly for this PR, so if anyone wants to do visual testing it will have to be done locally. This is how it should look:

rusackas · 2022-12-07T20:49:58Z

Ephemeral environment seems to be running OK, but I spotted a couple of styling issues there. I can't seem to upload an image here for some reason at the moment, so I'll Slack ya.

lyndsiWilliams · 2022-12-08T00:22:18Z

/testenv up FEATURE_ALERT_REPORTS=true

github-actions · 2022-12-08T00:26:34Z

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

rusackas · 2022-12-15T20:32:24Z

Do we still have build/ephemeral concerns here, or is this ready for the final once-over?

AAfghahi

Looks good!

lyndsiWilliams · 2022-12-16T18:50:54Z

Do we still have build/ephemeral concerns here, or is this ready for the final once-over?

I'm not sure what's going on with the ephemeral, I'd be happy to zoom and show this locally if that would increase confidence in this. I feel (and hope) that this is just a case of the ephemeral not working correctly.

eschutho · 2022-12-16T20:43:51Z

superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx

@@ -1044,14 +1051,50 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
    setIsHidden(false);
  }

+  const SAVE_TEXT = t('Save');


@lyndsiWilliams are we going to move these into a TRANSLATIONS object?

Definitely! Done in this commit.

…m/apache/superset into lyndsi/fix-spacing-in-alert-modal

eschutho

Looks great!

github-actions · 2022-12-20T17:28:23Z

Ephemeral environment shutdown and build artifacts deleted.

Fixed spacing in alert modal and translated untranslated strings

c86219c

lyndsiWilliams requested a review from eschutho November 8, 2022 19:32

pull-request-size bot added the size/M label Nov 8, 2022

pkdotson approved these changes Nov 8, 2022

View reviewed changes

Adjust styling in alert modal

120c352

Aligned all elements with 16px border

af49885

pull-request-size bot added size/S and removed size/M labels Dec 6, 2022

Antonio-RiveroMartnez reviewed Dec 6, 2022

View reviewed changes

Move translated text to variables

44a5883

pull-request-size bot added size/L and removed size/S labels Dec 7, 2022

lyndsiWilliams closed this Dec 7, 2022

lyndsiWilliams reopened this Dec 7, 2022

Add space between input labels

03447be

Use useTheme hook instead of supersetTheme

74e5fb5

lyndsiWilliams force-pushed the lyndsi/fix-spacing-in-alert-modal branch from 1c4743d to 74e5fb5 Compare December 8, 2022 00:45

lyndsiWilliams added 2 commits December 7, 2022 18:45

Merge branch 'master' into lyndsi/fix-spacing-in-alert-modal

c5a4f50

useTheme wasn't needed after all

aae2308

AAfghahi approved these changes Dec 16, 2022

View reviewed changes

eschutho reviewed Dec 16, 2022

View reviewed changes

lyndsiWilliams added 3 commits December 19, 2022 09:55

Move translated strings into a TRANSLATIONS object

c740394

Merge branch 'lyndsi/fix-spacing-in-alert-modal' of https://github.co…

b34e8e6

…m/apache/superset into lyndsi/fix-spacing-in-alert-modal

fix merge

f7f6303

eschutho approved these changes Dec 20, 2022

View reviewed changes

lyndsiWilliams merged commit 1bab5b9 into master Dec 20, 2022

lyndsiWilliams deleted the lyndsi/fix-spacing-in-alert-modal branch December 20, 2022 17:28

This was referenced Jan 3, 2023

chore: Migrate /superset/search_queries to API v1 #22579

Merged

chore: Migrate /superset/queries/<last_updated_ms> to API v1 #22611

Merged

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixed spacing in alert modal #22066

fix: Fixed spacing in alert modal #22066

lyndsiWilliams commented Nov 8, 2022

codecov bot commented Nov 8, 2022 •

edited

Loading

lyndsiWilliams commented Nov 8, 2022

github-actions bot commented Nov 8, 2022

eschutho commented Nov 8, 2022

rusackas commented Dec 5, 2022

lyndsiWilliams commented Dec 6, 2022 •

edited

Loading

lyndsiWilliams commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

Antonio-RiveroMartnez Dec 6, 2022

lyndsiWilliams Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022 •

edited

Loading

rusackas commented Dec 7, 2022

lyndsiWilliams commented Dec 8, 2022

github-actions bot commented Dec 8, 2022

rusackas commented Dec 15, 2022

AAfghahi left a comment

lyndsiWilliams commented Dec 16, 2022

eschutho Dec 16, 2022

lyndsiWilliams Dec 19, 2022

eschutho left a comment

github-actions bot commented Dec 20, 2022

fix: Fixed spacing in alert modal #22066

fix: Fixed spacing in alert modal #22066

Conversation

lyndsiWilliams commented Nov 8, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

AFTER (extended)

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Nov 8, 2022 • edited Loading

Codecov Report

lyndsiWilliams commented Nov 8, 2022

github-actions bot commented Nov 8, 2022

eschutho commented Nov 8, 2022

rusackas commented Dec 5, 2022

lyndsiWilliams commented Dec 6, 2022 • edited Loading

lyndsiWilliams commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

Antonio-RiveroMartnez Dec 6, 2022

Choose a reason for hiding this comment

lyndsiWilliams Dec 7, 2022

Choose a reason for hiding this comment

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

lyndsiWilliams commented Dec 7, 2022 • edited Loading

rusackas commented Dec 7, 2022

lyndsiWilliams commented Dec 8, 2022

github-actions bot commented Dec 8, 2022

rusackas commented Dec 15, 2022

AAfghahi left a comment

Choose a reason for hiding this comment

lyndsiWilliams commented Dec 16, 2022

eschutho Dec 16, 2022

Choose a reason for hiding this comment

lyndsiWilliams Dec 19, 2022

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2022

codecov bot commented Nov 8, 2022 •

edited

Loading

lyndsiWilliams commented Dec 6, 2022 •

edited

Loading

lyndsiWilliams commented Dec 7, 2022 •

edited

Loading