feat: blind sorting option for vislib #813
Conversation
nickofthyme
added
:data
nickofthyme
force-pushed
the
feat/vislib-sorting
branch
from
ddda457
to
1182969
Compare
| @@ -38,6 +38,8 @@ export const computeSeriesDomainsSelector = createCachedSelector( | |||
| customYDomainsByGroupId, | |||
| deselectedDataSeries, | |||
| settingsSpec.xDomain, | |||
| // @ts-ignore blind sort option for vislib | |||
| settingsSpec.vislibSort, | |||
There was a problem hiding this comment.
Though planned as temporary, let's use a name that tells what it is, even if it's longer, than what's otherwise a temporarily good name that'll become cryptic
| const x = getAccessorValue(datum, xAccessor); | ||
| // skip if the datum is not an object or null | ||
| if (typeof datum !== 'object' || datum === null) { | ||
| return null; |
There was a problem hiding this comment.
We're in a forEach, no need to return a value
| xValues.push(x); | ||
| // skip if the x value is not a string or a number | ||
| if (typeof x !== 'string' && typeof x !== 'number') { | ||
| return null; |
| }); | ||
| }); | ||
| } else { | ||
| data.forEach((datum) => { |
There was a problem hiding this comment.
There's a level of code duplication in the branch, it'd be OK to distinguish between the cases in a more fine grained way
|
|
||
| // skip if the datum is not an object or null | ||
| if (typeof datum !== 'object' || datum === null) { | ||
| return null; |
There was a problem hiding this comment.
Won't repeat comments on like things as they'd go away upon DRYing up the code
|
|
||
| xValues.push(x); | ||
|
|
||
| yAccessors.forEach((accessor, index) => { |
There was a problem hiding this comment.
The DRY approach would also favor code review as now it's a bit hard to contrast how the added logic deviates from the preexisting version
Codecov Report
@@ Coverage Diff @@
## master #813 +/- ##
==========================================
+ Coverage 74.33% 74.57% +0.23%
==========================================
Files 273 288 +15
Lines 9321 9627 +306
Branches 2009 2058 +49
==========================================
+ Hits 6929 7179 +250
- Misses 2384 2435 +51
- Partials 8 13 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@monfera I would normally agree with your approach to be DRY, by maybe pulling the logic into a separate function that handles the checks and adding the value to the Would that be ok with you? |
|
@nickofthyme thanks for the change, I'm fine with temporary non-DRYness there |
|
🎉 This PR is included in version 21.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [21.2.0](elastic/elastic-charts@v21.1.2...v21.2.0) (2020-09-14) ### Features * blind sorting option for vislib ([opensearch-project#813](elastic/elastic-charts#813)) ([79d0e4f](elastic/elastic-charts@79d0e4f)) * order ordinal values by sum ([opensearch-project#814](elastic/elastic-charts#814)) ([45ea0c6](elastic/elastic-charts@45ea0c6)) * **series:** add simple mark formatter ([opensearch-project#775](elastic/elastic-charts#775)) ([75bd8d5](elastic/elastic-charts@75bd8d5))
Summary
Allows a blind sorting option on
SettingscalledenableVislibSeriesSortto loop through allyAccessorsthen all the data.This option is bind as in not added to the
SettingSpecto avoid usage, outside of vislib, until #795 allows a sorting alternative.Screenshot
Sample playground
Checklist
src/index.ts(and stories only import from../srcexcept for test data & storybook)