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

compare feature for one buiding, multiple time periods #283

Merged
merged 48 commits into from
Jan 18, 2024

Conversation

solderq35
Copy link
Contributor

@solderq35 solderq35 commented Dec 12, 2023

Main TODO

Maybe should separate this out to separate GitHub Issues. Not exhaustive

  • "Cancel" button on edit menu was not tested very thoroughly for the timestamps, needs bug fixes
    • If you saved some timestamps earlier (clicked "save time period" > "ok"), then open edit menu again and then click "cancel", the counter on the "save time period" goes all the way to zero
  • Multiple time period chart bug where after comparing more than one time period of a single building, then going to the map page and comparing that same building with another will result in the extra time period chart(s) showing up. The chart is fixed on page refresh
  • Labels on the charts needs work, see comments on building_compare_mod.js file. Need to handle (both on first page load and after editing the chart):
    • One building, multiple time periods (priority)
    • One building, same time period
    • Multiple Buildings, same time period
      • Need energy type labels (pointstring) for multiple time periods graph, after we implement ability to change meters
  • Support different meters per building (say if you want to compare a particular meter on one building with multiple time periods)
    • If you look at the current edit_card.vue code for regular buildings, technically this is for meter groups
  • Support different energy types (Electricity, Steam. etc)
    • Changed to immediately take user to comparison page for single building comparison. Only electricity is supported for multiple building comparison
  • Implement moving the charts left to align by day of the month (So December 2 and October 2, for instance, start on same point on X-axis)
  • Implement tooltips (hovering mouse over graph at specific points) that are accurate to the actual dates. Might need to track the actual dates separately depending on how we move the charts left
    • See src\store\chart_modifiers\line_bar\accumulated_real.js for current implementation of tooltips
  • Add some kind of block UI for the edit menu, showing start and end dates for previously saved time periods (refer to Figma). Should support deleting time periods from the edit menu UI, and maybe editing previously submitted time periods as well
    • Block UI with start and end dates for previously saved time periods
    • Delete functionality. X button per saved time block. No edit functionality needed.
  • Add a link to the compare menu for same building, multiple time periods from the individual building (non compare) pages
  • (Stretch goal): Check for mobile compatibility before merging, due to 600px media queries for mobile screens. Especially on map_prompt.vue file

Issue

#278

@solderq35
Copy link
Contributor Author

solderq35 commented Dec 19, 2023

TODO:

  • Support multiple charts of the same building properly (not just the hacky way it's currently supported by block.module.js's loadDefault function)
  • Probably need to rework the whole block / chart data structure to support multiple time periods per chart. May require backend work or updating API, not sure
  • It may actually be more efficient to directly add new time fields to edit_chart.vue, as there are CORS / security errors trying to change the block's dateStart / dateEnd directly in the JS files
  • Fix bug with chart labels resetting after you change the time period with edit menu
    image

@solderq35
Copy link
Contributor Author

solderq35 commented Dec 19, 2023

chart labels issue:

// line below is what "resets" the chart name, maybe comment it out but needs more testing

I think the reason this changes the chart label to energy type after editing, is to support the regular (non comparison) graphs, to make it clear to the user which energy type is which. So don't comment out or change this line until we can account for different use cases of comparison and non-comparison.

@solderq35
Copy link
Contributor Author

solderq35 commented Dec 20, 2023

Before

image

After (data shifted left)

image

Was able to figure out how to properly align data from different months.

chart.module.js file makes API call to get data via meter_Groups.module.js

So as far as just getting the data to be from different time periods we can do all of that in chart.module file in theory (although it may be better in the long run to rework block.module.js).

Just need to figure out how to support more than 3 charts without vuex error, and how to connect the different time settings in chart.module.js to the edit_card form

@solderq35
Copy link
Contributor Author

solderq35 commented Dec 24, 2023

Screenshots

image

image

image

image

Instructions

Go to a comparison URL with just one building in it, e.g. http://localhost:8080/#/compare/%5B%2216%22%5D/2 (or http://localhost:8080/#/compare/%5B"16"5D/2, as " renders as %22 in URL)

It is probably better to use the "one building in URL" approach, as if you had to change the URL every time you added a time period, that might force the page to refresh and cause unexpected behaviors.

As shown in above screenshots, adjust start and end dates and then click "Save Time Period". Repeat for all time periods you want to save, then click "OK" at the end.

@s-egge
Copy link
Member

s-egge commented Dec 24, 2023

Before

image

After

image

image

image

I was able to get the data shifted left by mapping the x-values of the charts to the x-values of the largest chart, similar to what was done before in chart.module but now it's done in chartController.vue.

The original x-values of the charts are saved in a new variable in their data so that they can be used in linechart.js in order to display the proper date when hovering.

image

The x-axis labels, bottom chart label, and line labels still need to be fixed, along with the issues mentioned above in the TODO.

@s-egge
Copy link
Member

s-egge commented Dec 26, 2023

Compare Multiple Time Periods Button

image
image

After Pressing The Button

image
Notice the change in URL between the first/second/third picture. After pressing the button, the multiple time period option shows up in the edit block.

Bugs

The buttons on the right of the navbar are still visible until the page is refreshed:
image
I've tried a few things to fix this (watching $route, using beforeRouteUpdate, etc.). The only thing that worked was moving the v-if="!otherView" from the menu tag to the bar tag above it. I didn't leave this in as I'm not familiar with the public/users views being referenced here:

After refreshing the page and pressing the back button in the browser (both with and without moving the v-if statement), the navbar is no longer visible on the building page until it is refreshed:
image

@s-egge
Copy link
Member

s-egge commented Dec 26, 2023

Before

image

After

image
image

The time period comparison option will only display if there are less than two buildings selected (so 0 or 1 building).

image

The compareStories array is passed to map_prompt.vue via props in order to check the number of buildings selected. I set the command to 't' but it isn't currently needed to be checked, since the else statement in map.vue's showComparison function handles the logic correctly whether it's one building or several.

s-egge and others added 2 commits December 26, 2023 16:18
Moved mgID outside the if-statement, otherwise no error was caught when attempting to compare multiple time periods on buildings with no electricty data.
Also added a check in navdir.vue so that the compare multiple time periods button won't show up if the building has no electricity data.
@solderq35
Copy link
Contributor Author

solderq35 commented Dec 28, 2023

The buttons on the right of the navbar are still visible until the page is refreshed:

After refreshing the page and pressing the back button in the browser (both with and without moving the v-if statement), the navbar is no longer visible on the building page until it is refreshed:

The issue was that there were conflicting instructions from the view.vue file vs. the navdir.vue file, and also the view.vue's navVis variable was only set on initial page load (async created...).

For now I set the view.vue's navVis variable to update its value whenever the page URL changes (reference in watch of view.vue file).

See comments of commit for more details. Other minor changes are to address console warnings.

@solderq35
Copy link
Contributor Author

Multiple time period label update

image image

I updated the labels at the top of the multiple time period comparison chart and added a check to remove the bottom label if there is more than one chart for a single building. The x-axis labels still need to be updated.

I also moved the majority of the formatting logic for multiple time periods into one function since it was getting large and starting to clutter the updateChart function

For the second image there (multiple time periods, one building), I would suggest adding energy types, so that the labels go something like "October 1 to November 1, Net Energy Usage)" (example), in order to support multiple meters (and meter types) in the future.

@s-egge
Copy link
Member

s-egge commented Jan 12, 2024

Compare Meters Other Than Electricity and Measurements other than Total

image

image

It's now possible to change the Meter type and Measurement. If time periods are added and the Meter/Measurement is changed, all data will be pulled based on whatever the Meter/Measurement is when "Ok" is hit. Different Meter/Measurement combos cannot be compared to each other right now.

image

The card name will now also change based on the Meter type, this updates when in both compare view and single building view.

@solderq35
Copy link
Contributor Author

solderq35 commented Jan 16, 2024

Stuff we didn't implement / maybe for future (include in email to Brandon and Ross)

  • Will you ever need to support non-electric energy types for multiple building comparison (e.g. compare 2 steam buildings)? Same question for multiple meters or measurements for multiple building comparison
  • Do you need any part of navdir component (share button, download button, etc.) on comparison page?
  • Update https://dashboard.sustainability.oregonstate.edu/#/getstarted regarding comparison feature?
  • Are the multiple rows of dates ok for the multiple time periods graph?
  • Will you ever need to support map screen popup for single building comparison?
    • I think brandon and ross said no, during meeting
  • Any other suggestions?

minor (won't fix)

  • "Total Electricity"
    • probably not important as the measurements are also shown on left side of chart

@solderq35
Copy link
Contributor Author

solderq35 commented Jan 16, 2024

TODO:

  • Remove any code comments that are just about this PR and not something we need in the codebase permanently
  • Remove any console.log statements that weren't already there
  • Check for any other bugs
  • reset env.development

Then we can merge PR and send email to Brandon / Ross with screenshots

@solderq35 solderq35 marked this pull request as ready for review January 16, 2024 20:12
@solderq35 solderq35 requested a review from s-egge January 16, 2024 20:13
Copy link
Member

@s-egge s-egge left a comment

Choose a reason for hiding this comment

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

The changes look good to me, I don't see any extra console.logs, irrelevant comments, or bugs.

@s-egge s-egge merged commit 105167b into master Jan 18, 2024
3 checks passed
@solderq35 solderq35 changed the title [Draft, do not merge] test multiple time periods compare feature for one buiding, multiple time periods Jan 18, 2024
@s-egge s-egge deleted the multiple-time-periods branch July 1, 2024 19:03
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