Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readded buttons and removed unnecessary buttons #1329

Merged
merged 13 commits into from
Nov 28, 2024

Conversation

williamw04
Copy link
Contributor

@williamw04 williamw04 commented Aug 11, 2024

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

  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

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.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@huss
Copy link
Member

huss commented Aug 12, 2024

For the record, I merged development so the DB is the latest.

@williamw04 williamw04 marked this pull request as ready for review October 11, 2024 15:11
@williamw04
Copy link
Contributor Author

Implemented advanced button feature to all graphics.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
src/client/app/components/LineChartComponent.tsx Outdated Show resolved Hide resolved
src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
src/client/app/components/LineChartComponent.tsx Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Nov 2, 2024

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 npm i [email protected] @types/[email protected] @types/[email protected] but this did not resolve it (actually there are now two warnings). It seems that the packages are old enough not to include recent changes.

There are ways to stop creating the source map but I did not try and had mixed feelings.

Does anyone have other thoughts/ideas?

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
src/client/app/components/LineChartComponent.tsx Outdated Show resolved Hide resolved
src/client/app/containers/CompareChartContainer.ts Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Nov 28, 2024

The warning is not going away so going to live with it for now. I'm leaving this comment so people know about it.

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @williamw04 for the changes and sorry I missed this was ready for review. Everything now looks good. Congratulations on another task finished.

@huss huss merged commit c7c25cf into OpenEnergyDashboard:development Nov 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants