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

Add date range to data.json #98

Closed
wants to merge 4 commits into from
Closed

Add date range to data.json #98

wants to merge 4 commits into from

Conversation

dcjohnson24
Copy link
Collaborator

Description

Checklist

  • I am requesting to merge into the dev branch
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Screenshots

Before After

Fixes #97

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for creative-quokka-61f1f5 ready!

Name Link
🔨 Latest commit cad1fd1
🔍 Latest deploy log https://app.netlify.com/sites/creative-quokka-61f1f5/deploys/64c98732423ae30008b5422c
😎 Deploy Preview https://deploy-preview-98--creative-quokka-61f1f5.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 31, 2023

Deploy Preview for sprightly-muffin-751ffc ready!

Name Link
🔨 Latest commit cad1fd1
🔍 Latest deploy log https://app.netlify.com/sites/sprightly-muffin-751ffc/deploys/64c987326a5e160008b1bb52
😎 Deploy Preview https://deploy-preview-98--sprightly-muffin-751ffc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hkier
Copy link
Collaborator

hkier commented Jul 31, 2023

This PR will adversely impact the changes I am working on for Issue #66 which are being worked upon in branch Decouple-data-part-1. In fact, it may actually be easier to decouple the data first and then implement this change. No matter how we do this, the changes will need to be coordinated.

@hkier hkier added do-not-merge waiting on additional commits blocked labels Jul 31, 2023
Copy link
Collaborator

@hkier hkier Jul 31, 2023

Choose a reason for hiding this comment

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

This filename name is a PITA. It will be difficult to type with both _ and - in the name. It also is visually tough to decipher. Perhaps it should be renamed something like, data-20220520-20230702. json

@lauriemerrell
Copy link
Member

I wonder if it might be easier to add some kind of date labeling within data.json itself, for example, adding an updated timestamp within a metadata tag in the file, or as an attribute on each row?

@hkier
Copy link
Collaborator

hkier commented Jul 31, 2023

In light of Laurie's comments, I've realized putting the date in the file name itself will force us to update the code every time the file is update with a new date range. This is the opposite of our end goal to decouple the data from the code. As it stands now, I would have to vote against this solution. The date range must be in the file data itself. It can either be on each record or a different element within the data structure. [date1, date2, routedata]

@dcjohnson24
Copy link
Collaborator Author

Thanks for the comments. I'll remove the date from the filename and add another field to data.json with the start and end date.

KyleDolezal
KyleDolezal previously approved these changes Aug 1, 2023
src/Routes/data.json Outdated Show resolved Hide resolved
@hkier
Copy link
Collaborator

hkier commented Aug 1, 2023

Is there an associated backend update which needs to be merged before this PR is merged? How is the date information being populated in the data.json file?

@hkier hkier dismissed KyleDolezal’s stale review August 1, 2023 23:19

There are still unanswered questions about this change.

@dcjohnson24
Copy link
Collaborator Author

There isn't a backend update yet, but I'm not sure it would affect this PR.
The date information would be taken from the beginning and end dates of the DataFrame storing the latest trip ratio information and saved to data.json, which is then passed to the frontend.

@lauriemerrell
Copy link
Member

This feature got added elsewhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked do-not-merge waiting on additional commits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Frontend] Add start and end date fields to data.json
4 participants