[ENG-6368] Share summary graphs #2361
Conversation
| cardSearchFilter: { | ||
| affiliation: this.args.model.institution.iris.join(','), | ||
| }, | ||
| sort: '-relevance', |
There was a problem hiding this comment.
as implemented, index-value-search ignores the sort param (always sorts by most-used to least-used) so you can leave this out (looks like the api docs are wrong about this, oops)
| return storageRegionsData; | ||
| } | ||
| }; | ||
| this.kpiCharts[index] = kpiChartObject; |
There was a problem hiding this comment.
this is getting a bit convoluted and brittle, overwriting an object in an array by constant index (especially when title and chartType are unchanged but computed two ways) and then re-rendering everything that depends on that array (multiple times, it seems?)
i gather you don't want to make the whole set of charts block on these value-search requests, which makes sense... might simplify things if the ChartKpiWrapper::ChartKpi component understood the task instance you're passing it and automatically used this.args.data.taskInstance.value (when this.args.data.chartData is missing) -- then this task could have just one argument (propertyPath), return its chartData, and not have to do anything with this.kpiCharts
There was a problem hiding this comment.
Ahh, this is a much more sensible way to implement this. Will rework these components now
| rawData = taskInstance.value; | ||
| } | ||
| } | ||
| rawData.map((rawChartData: ChartDataModel, $index: number) => { |
There was a problem hiding this comment.
nit (from prior code): .forEach is more appropriate than .map if you're not returning values (this creates an array that's immediately discarded)
| let rawData = chartData ? chartData : []; | ||
| if (taskInstance) { | ||
| if (taskInstance.value) { | ||
| rawData = taskInstance.value; | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: this is harder to read than it needs to be; would be clearer condensed
const rawData = taskInstance?.value || chartData || [];| chartType: string; | ||
| // Either chartData or taskInstance should be defined | ||
| chartData?: ChartDataModel[]; | ||
| taskInstance?: TaskInstance<void>; |
There was a problem hiding this comment.
| taskInstance?: TaskInstance<void>; | |
| taskInstance?: TaskInstance<ChartDataModel[]>; |
futa-ikeda
merged commit faa9268
into
CenterForOpenScience:feature/insti-dash-improv
…ForOpenScience/ember-osf-web into add-fancy-pagination-buttons * 'feature/insti-dash-improv' of https://github.com/CenterForOpenScience/ember-osf-web: [ENG-6246][ENG-6269][ENG-6275] Add new property fields to mirage (#2353) [ENG-6368] Share summary graphs (#2361) Update month data fields to be strings (#2358) make total user count dynamic for the user tab # Conflicts: # translations/en-us.yml
Purpose
Summary of Changes
Screenshot(s)
Finished graphs:
Loading/error states:
Side Effects
QA Notes