Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: update chart sorting control labels/descriptions #1436

Merged
merged 1 commit into from
Oct 28, 2021
Merged

fix: update chart sorting control labels/descriptions #1436

merged 1 commit into from
Oct 28, 2021

Conversation

john-bodley
Copy link
Contributor

@john-bodley john-bodley commented Oct 26, 2021 •
edited
Loading

🏠 Internal

This PR is one of a slew to the apache/superset and apache-superset/superset-ui repos to revert pseudo recent changes to the series restrictions for high cardinality groupings. For more context please refer to this Slack thread in the #committers channel.

Specifically this PR updates the Group by, Sort by, Series limit, and Row limit descriptions to provide more context, recommended behaviors, and standardizes the language.

Related PRs:

@vercel
Copy link

vercel bot commented Oct 26, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/CrWYdoDmoNxAaSWpSyARM6Grxkt6
✅ Preview: https://superset-ui-git-john-bodley-series-descriptions-superset.vercel.app

@john-bodley john-bodley force-pushed the john-bodley--series-descriptions branch from 0bd0b45 to e5f03ba Compare October 26, 2021 16:22
@john-bodley john-bodley mentioned this pull request Oct 26, 2021
Merged
@john-bodley john-bodley changed the title docs: update chart sorting control labels/descriptions fix: update chart sorting control labels/descriptions Oct 26, 2021
@john-bodley john-bodley force-pushed the john-bodley--series-descriptions branch from e5f03ba to 21c3593 Compare October 26, 2021 18:12
@codecov
Copy link

codecov bot commented Oct 26, 2021 •
edited
Loading

Codecov Report

Merging #1436 (b68a49d) into master (f279549) will not change coverage.
The diff coverage is n/a.

❗ Current head b68a49d differs from pull request most recent head 5e68eef. Consider uploading reports for the commit 5e68eef to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1436   +/-   ##
=======================================
  Coverage   30.42%   30.42%           
=======================================
  Files         497      497           
  Lines       10003    10003           
  Branches     1689     1689           
=======================================
  Hits         3043     3043           
  Misses       6714     6714           
  Partials      246      246
Impacted Files Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx 37.83% <ø> (ø)
...et-ui-chart-controls/src/shared-controls/index.tsx 36.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f279549...5e68eef. Read the comment docs.

@john-bodley john-bodley mentioned this pull request Oct 26, 2021
Merged
9 tasks
@john-bodley john-bodley marked this pull request as ready for review October 26, 2021 19:26
@john-bodley john-bodley requested a review from a team as a code owner October 26, 2021 19:26
john-bodley
john-bodley commented Oct 26, 2021
View reviewed changes
packages/superset-ui-chart-controls/src/shared-controls/index.tsx
@@ -333,10 +337,11 @@ const limit: SharedControlConfig<'SelectControl'> = {
default: 100,
choices: formatSelectOptions(SERIES_LIMITS),
description: t(
'Limits the number of time series that get displayed. A sub query ' +
Copy link
Contributor Author

@john-bodley john-bodley Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The TL;DR is I removed the term time for consistency and the direction @villebro is moving, renamed dimension to column, and added a cost comment.

john-bodley
john-bodley commented Oct 26, 2021
View reviewed changes
packages/superset-ui-chart-controls/src/shared-controls/index.tsx Outdated
),
};

const sort_by: SharedControlConfig<'MetricsControl'> = {
type: 'MetricsControl',
label: t('Sort By'),
label: t('Series sort by'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Striving for consistency (in a somewhat inconsistent world), i.e., other labels are of the form Group by and not Group By.

@john-bodley john-bodley force-pushed the john-bodley--series-descriptions branch from b68a49d to 5e68eef Compare October 26, 2021 23:01
@john-bodley
Copy link
Contributor Author

john-bodley commented Oct 26, 2021 •
edited
Loading

Thanks @kgabryje and @ktmud for the review. Based on learnings from https://github.com/apache/superset/pull/17236—where enforcement is difficult—I updated the language to be recommendations, i.e., should rather than must.

I also discovered that the Sort by widget applies to the table visualization and thus the Series sort by label was overly prescriptive and thus kept the label unchanged.

@john-bodley john-bodley merged commit b79168a into master Oct 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the john-bodley--series-descriptions branch October 28, 2021 15:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kgabryje kgabryje kgabryje left review comments

@ktmud ktmud ktmud approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@john-bodley @ktmud @kgabryje