Convert ResultAggregator widget #572
Conversation
| getResultData () { | ||
| this.setState({isLoading: true}); | ||
| HttpClient.get([Settings.serverUrl, 'widget', 'result-aggregator'], this.params) | ||
| const getChartData = useCallback(() => { |
There was a problem hiding this comment.
I think you can simplify things here a bit, given the use of this callback within the useEffect hook and the two functions that mutate props.params.
This is also used by WidgetHeader to place a refresh function, but I don't believe that callback or refresh button is necessary with the proper lifecycles for the components. Since the dashboard pages are now statically pathed, a browser refresh can be used. This refresh was necessary when a browser refresh would take you back to the default dashboard.
Consider:
This function should be a useEffect, with dependencies on [params], making the HttpClient call only when params is not empty.
With this, the hook will run on initial load when params gets set, and then will also be triggered by the two functions mutating params automatically.
There was a problem hiding this comment.
Best I could get is a dependency array of params, days and groupFields. It is not awesome code though.
Issue is, if we set params as only dependency then we either:
- Modify inner fields (e.g
params.<field> = value) which won't modify the object, thus,useEffectwill not run. - Create a new
paramsobject (e.g.params = {...params, <field>: value}which will fail because params is read-only
|
|
||
| return ( | ||
| <Card> | ||
| <WidgetHeader title={title} getDataFunc={getChartData} onEditClick={onEditClick} onDeleteClick={onDeleteClick}/> |
There was a problem hiding this comment.
If you move getChartData to a direct useEffect hook, this prop would be removed and WidgetHeader just won't render a refresh button.
| <p>No data returned, try changing the days.</p> | ||
| } | ||
| {(!resultAggregatorError && isLoading) && | ||
| <Text component="h2">Loading ...</Text> |
There was a problem hiding this comment.
Consider <Title headingLevel='h2'>
| tooltip="Group data by:" | ||
| /> | ||
| <ParamDropdown | ||
| dropdownItems={[0.1, 0.5, 1, 3, 5]} |
There was a problem hiding this comment.
I'd like to actually expand these options, since 5 days is often insufficient.
@LightOfHeaven1994 thoughts on adding an option for 10 or 14 days here?
There was a problem hiding this comment.
I went ahead and added some values that could fit as testing
No description provided.