Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: replace "dashboard" -> "report" in chart email report modal #25540

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

sfirke
Copy link
Member

@sfirke sfirke commented Oct 5, 2023

SUMMARY

When setting up a new email report for a chart, the popup incorrectly says "dashboard" and "screenshot."

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

In Superset 3.0.0, click the ... on a chart, then Manage email report, then Set up an email report. You'll see:

image

a) This shouldn't say screenshot because the user can pick a text or .CSV format choice
b) This should say chart instead of dashboard

TESTING INSTRUCTIONS

N/A, just changing a line of text.

@sfirke
Copy link
Member Author

sfirke commented Oct 5, 2023

I'm confused, what does this failed check say I should do?

Error: 337:14 error Replace ⏎············{t('The·report·will·be·sent·to·your·email·at')}⏎·········· with {t('The·report·will·be·sent·to·your·email·at')} prettier/prettier

EDIT: Oh of course it was the whitespace around it

@sfirke
Copy link
Member Author

sfirke commented Oct 5, 2023

Also I see that the string I modified A screenshot of the dashboard will be sent to your email at appears in ten translation files when I search the code base. Should I make the corresponding modification in each of those as well?

@sfirke
Copy link
Member Author

sfirke commented Mar 22, 2024

Ugh I am bad at rebasing! 😖

@sfirke sfirke closed this Mar 22, 2024
@github-actions github-actions bot added i18n Namespace | Anything related to localization risk:db-migration PRs that require a DB migration i18n:spanish Translation related to Spanish language i18n:italian Translation related to Italian language i18n:french Translation related to French language i18n:chinese Translation related to Chinese language i18n:japanese Translation related to Japanese language i18n:russian Translation related to Russian language i18n:korean Translation related to Korean language api Related to the REST API labels Mar 22, 2024
@github-actions github-actions bot removed i18n:chinese Translation related to Chinese language i18n:japanese Translation related to Japanese language i18n:russian Translation related to Russian language i18n:korean Translation related to Korean language api Related to the REST API doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages i18n:dutch i18n:slovak i18n:ukrainian i18n:portuguese i18n:brazilian labels Mar 22, 2024
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.09%. Comparing base (0c40bea) to head (e4dcb53).
Report is 2214 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #25540      +/-   ##
==========================================
+ Coverage   68.51%   69.09%   +0.57%     
==========================================
  Files        1915     1919       +4     
  Lines       75100    74969     -131     
  Branches     8314     8349      +35     
==========================================
+ Hits        51455    51800     +345     
+ Misses      21503    21118     -385     
+ Partials     2142     2051      -91     
Flag Coverage Δ
javascript 57.40% <ø> (+1.08%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sfirke
Copy link
Member Author

sfirke commented Mar 22, 2024

@rusackas I rebased this one and it's passing CI. It's not at all important but I'm pinging you in case you want to reduce the open count by one. I confirmed that this language is currently in 4.0.0rc1 and still wrong.

Updating the English makes the translations slightly out of date, I don't know how to deal with that...

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose my only little question is this: does a user reliably know that a Report is a screenshot of a dashboard? I would assume so, but I'm not sure if this is explained elsewhere in the UI. I'll approve, since it seems fine to me, but I'll also ping @yousoph and @eschutho to see if they have feelings about it :D

@sfirke
Copy link
Member Author

sfirke commented Mar 22, 2024

I suppose my only little question is this: does a user reliably know that a Report is a screenshot of a dashboard?

Ah, I sent this PR because I was setting up a Report for a single chart and didn't like that it said "dashboard." I see now that when I set up a report for a dashboard, it uses this same modal, and in that case "dashboard" and "screenshot" are correct.

Maybe that's where you're coming from? A report is not always a screenshot of a dashboard, it can be a screenshot or .csv of a single chart. Thus this fix which makes it agnostic as to which is being configured.

@sfirke sfirke merged commit 0df7832 into apache:master Mar 22, 2024
30 checks passed
@sfirke sfirke deleted the report-modal-fix branch March 22, 2024 19:33
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 4.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants