-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat(viz-gallery): add 'feature' tag and fuzzy search weighting #18662
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18662 +/- ##
=======================================
Coverage 66.51% 66.51%
=======================================
Files 1644 1646 +2
Lines 63491 63530 +39
Branches 6458 6466 +8
=======================================
+ Hits 42231 42258 +27
- Misses 19590 19596 +6
- Partials 1670 1676 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx
Outdated
Show resolved
Hide resolved
…ol/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
…ol/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
…ol/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
…ol/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io>
Pinging @suddjian for a review since he's the SME for the fuzzy search aspect. |
@stephenLYZ @yousoph @kasiazjc Let's test an ephemeral environment here (when CI is finished) and then we can probably remove the labels from the plugins here (just leaving the functionality for this PR). We want to leverage this mainly for Preset purposes, so we can configure it there. @graceguo-supercat @amitmiran137 (or anyone looking at this):
|
/testenv up |
@geido Ephemeral environment spinning up at http://34.217.75.91:8080. Credentials are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@stephenLYZ This is looking great! I know this sounds weird, but let's take the metadata out of the charts for now, so we can merge the feature code of this PR. Then we at Preset (or any other org) can add the labels we want, and not press anything upon the community quite yet. We can bring this up for wider discussion as a feature, to gain broader consensus around which charts are worth highlighting for the OSS releases. |
label: { | ||
name: t('verified'), | ||
description: t( | ||
'This chart was tested and verified, so the overall experience should be stable.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that a label is very flexible, but do we actually want such high flexibility? If this were, say, an enum of VERIFIED
, DEPRECATED
, ... , it could be used for more programmatic things. For example, rather than using a searchWeight that has to be the same for all verified charts, the logic in the gallery's sort function could sort by the value of this enum. And having this text duplicated in every verified
plugin doesn't really seem ideal to me.
We already have a boolean field deprecated
, so if we did switch this to an enum we would probably want to... deprecate the deprecated field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this inputs!This makes sense. @rusackas I think we can unify it into an enum and influence the search results based on the definition of this field.
…to feat-label-search
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/index.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/index.ts
Outdated
Show resolved
Hide resolved
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.222.147.29:8080. Credentials are |
More screenshots and implementation examples to add to the thread for posterity: And to implement:
label: {
name: ChartLabel.VERIFIED, // or others from the ChartLabel enum
description: t(
'This chart was tested and verified, so the overall experience should be stable.',
),
},
|
Future considerations:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left some feedback for follow-up considerations, and cleared out the example implementations so that we can merge this without making assumptions of what people will want to highlight (for now, anyway).
@rusackas Thanks for these suggestions! I agree to put this chat label into the weighted calculation of Fuse, which I tried but it did not succeed. The reason is that fuse calculates the score based on the key (https://github.com/krisk/Fuse/blob/master/src/core/computeScore.js), like 'name' or 'description', but in our scenario, it is based on the value, like 'verified' or 'feature'. There is one approach I have in mind is to use: make this label as key, and value is the same as name metadata: {
name: t('Pie Chart'),
verified: t('Pie Chart'),
// or
featured: t('Pie Chart')
} So we can define the weights of keys: new Fuse(chartMetadata, {
ignoreLocation: true,
threshold: 0.3,
keys: [
{
name: 'value.verified',
weight: 1.1
},
{
name: 'value.featured',
weight: 0.9
},
'value.name',
'value.tags',
'value.description'
],
}),
But this is not flexible. So there is a balance here. |
Yeah, I can't think of a clear way to have these numeric weights influence the text-based weighted keys of fuse, either. Let's consider that for a follow-up, I think this PR moves the ball in the right direction far enough for now. |
Ephemeral environment shutdown and build artifacts deleted. |
…ng (apache#18662)" This reverts commit 7524e1e.
…he#18662) * feat(viz-gallery): add 'feature' tag and fuzzy search weighting * add search weight * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * some improvements * take metadata out * use chartLabel enum to unify * add test chart * fix cache * Resolving TS Lint issue * Appeasing the linter * Removing one example implementation * Removing another example label implementation * Removing the third example label implementation Co-authored-by: Evan Rusackas <evan@preset.io> (cherry picked from commit 7524e1e)
🏷️ preset:2022.11 |
* feat(viz-gallery): add 'feature' tag and fuzzy search weighting * add search weight * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * Update superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx Co-authored-by: Evan Rusackas <evan@preset.io> * some improvements * take metadata out * use chartLabel enum to unify * add test chart * fix cache * Resolving TS Lint issue * Appeasing the linter * Removing one example implementation * Removing another example label implementation * Removing the third example label implementation Co-authored-by: Evan Rusackas <evan@preset.io> (cherry picked from commit 7524e1e)
SUMMARY
This PR add "verified" label to let the user know which chart types are more stable/tested/bug-free than others. And add search weight to effect search result.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
2022-02-11.12.33.52.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION