Readded buttons and removed unnecessary buttons #1329
Conversation
There was a problem hiding this comment.
Thanks to @williamw04 for addressing this issue. Code review found no issues and it works as expected. I think this can sit for a little while to see if someone can figure out how to hide/show the advanced options until a button is pushed (or something similar). If not, then I think this can be merged and leave that as an open issue.
|
For the record, I merged development so the DB is the latest. |
|
Implemented advanced button feature to all graphics. |
There was a problem hiding this comment.
Thanks to @williamw04 for figuring out how to make the icons dynamic on click. Overall, it works well. I have made a few comments in files. Also, I noticed that maps is not done. I don't recall if that was by design or because maps were not converted to the latest Redux. Right now I'm thinking it would be nice. If you agree, maybe a comment on PR #1314 that is updating React usage for maps with the desired buttons for each case would be good. Then it could be added to that.
There was a problem hiding this comment.
Thanks to @williamw04 for the updated code. I've made a few comments to address. Also, I am getting a warning on compile of:
web-1 | Failed to parse source map from '/usr/src/app/node_modules/@plotly/mapbox-gl/dist/mapbox-gl-unminified.js.map' file: Error: ENOENT: no such file or directory, open '/usr/src/app/node_modules/@plotly/mapbox-gl/dist/mapbox-gl-unminified.js.map'
I don't think it was there before. I wonder if it relates to the change in the includes (but I have not looked at it).
I tried to resolve this. I found this plotly GitHub issue that might have been related (for different package). It was resolved very recently. I don't know what it means for webpack given the shift in plotly compiles. I tried upgrading plotly packages to the latest with There are ways to stop creating the source map but I did not try and had mixed feelings. Does anyone have other thoughts/ideas? |
There was a problem hiding this comment.
Thanks to @williamw04 for the changes. I've made a few comments to address. I also added a comment on the compile warning. It is not resolved but not sure what to do.
src/client/app/translations/data.ts
Outdated
| @@ -442,6 +442,7 @@ const LocaleTranslationData = { | |||
| "TimeSortTypes.meter": "meter value or default", | |||
| "today": "Today", | |||
| "toggle.link": "Toggle chart link", | |||
| "Toggle Option" : "Toggle Option", | |||
There was a problem hiding this comment.
The key should be lowercase and words separated by a period so it is "toggle.option". Also, the text should be "Toggle Options" (pluralized).
| modeBarButtonsToRemove: listOfButtons, | ||
| modeBarButtonsToAdd: [{ | ||
| name: 'toggle-options', | ||
| title: translate('toggle option'), |
There was a problem hiding this comment.
In this and all similar files this should be toggle.option for the translate key.
Since PR #1354 was merged, component functions should replace translate with useTranslate. This also applies to all related files.
| displayModeBar: false, | ||
| displayModeBar: true, | ||
| modeBarButtonsToRemove: defaultButtons, | ||
| // TO DO: Removes line above and uncomment below. Read above for more info |
Description
There was a desire to use plotly buttons since there were certain desired features such as saving a png of the chart. However, enabling buttons displayed too many buttons and undesired features.
Partly Addresses #1160 & #1161
I enabled buttons on all chart component and removed buttons that doesn't correctly interact with OED, redundant or isn't useful.
Type of change
Checklist
Limitations
There is no option/button to display more buttons (advance buttons). The normal user will only use the download png button (downloads a png of a graph).
To Do:
Add a button to display advance buttons. In this case users should see a download png button, the plotly icon and the advance feature button when loading up a graph and clicking the advance feature button should display the advance buttons.