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.
There was a problem hiding this comment.
Thanks to @danielshid for addressing the previous comments. Review and testing found it works as desired. I have one some comment on a possible change to the code that I wanted your input on. If you like it then you are welcome to make the change.
| React.useEffect(() => { | ||
| // If user is in custom input mode, don't reset to standard options | ||
| if (!isCustomInput) { | ||
| const isCustom = !['1', '7', '28'].includes(duration.asDays().toString()); |
There was a problem hiding this comment.
To fully integrate looping over the enum for duration, what if this line is replaced with:
const durationValues = Object.values(ComparePeriod) as string[];
const isCustom = !(durationValues.includes(duration.asDays().toString()));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