Issue#1163 resolved, Issue#1292 resolved, and Issue#1293 partially resolved #1325
Conversation
|
This PR has merge conflicts. It would help me to have them resolved so I can test the final result and run the tests. If you need help then let me know. Thanks. |
|
I think this only partly addresses issue #1293 since it does not do compare. I think the description should be modified to reflect this so it will not automatically close when this PR is merged. Also, adding a comment to the issue that map is complete would be good. |
There was a problem hiding this comment.
Thanks to @danielshid for addressing all these issues. Testing found it works as expected. I made a couple of comment involving code layout to think about. Let me know if you have thoughts or want to discuss. I also made a comment about one linked issue.
danielshid
changed the title
There was a problem hiding this comment.
Thanks to @danielshid for the updated code. Review found it works as desired. I've made a few small comments to address but this is very close to done.
Also, I merged development to test. You will need to update your DB per the msg to Discord developers.
There was a problem hiding this comment.
Thanks to @danielshid for addressing the previous comments. I've made a couple more that should be fairly easy to address. Let me know if you have questions or cannot address them.
There was a problem hiding this comment.
Thanks to @danielshid for the updated code. While they addressed the comments at the time, I've made a few more based on other code coming and some things I noticed. I appreciate you looking into them.
| )) | ||
| : ( | ||
| <> | ||
| <option value='1'>{translate('day')}</option> |
There was a problem hiding this comment.
Why not use comparePeriodTranslations? Another comment might naturally lead to this.
| }; | ||
|
|
||
| // Defines the set of options for the dropdown | ||
| const options = chartType === ChartTypes.compare |
There was a problem hiding this comment.
I was thinking that the menu would be set up with the three standard option and the custom added after the loop when appropriate. Here is code from an upcoming change to units that shows the basic idea:
{Object.entries(LineGraphRates).map(
([rateKey, rateValue]) => (
<option value={rateValue * 3600} key={rateKey}>
{translate(rateKey)}
</option>
)
)}
<option value={CUSTOM_INPUT}>
{translate('custom.value')}
</option>What do you think?
Description
I fixed the issue with map displaying the wrong data based on bar by implementing it so bar and map are dependent on the same range. I also implemented dropdown menus for bar, map, and compare intervals, as well as the custom option for map. Map is complete; however, custom cannot currently be implemented for compare.
Fixes #1163, Fixes #1292, and Partly Addresses #1293
Type of change
Checklist
Limitations
N/A