-
Notifications
You must be signed in to change notification settings - Fork 5
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
Lowercase all data keys on consumption #32
Comments
Do we want to change only the "Date" keys (in getDailyStatistics() and used in XYGraph) to lowercase or do we want to change literally ALL of the keys returned by ALL API calls? Changing all of the keys to lowercase breaks the app. We'd have to refactor quite a bit beyond just removing the dateCap related code. Which is doable, but I don't want to start changing a bunch of stuff that doesn't need to be changed if that wasn't the intent. I currently have the following code in utilities.js
I have also updated getDailyStatistics and XYGraph.js to utilize the new 'data' key instead of dateCap and dateKey. If we need to go beyond that, let me know and I'll do the rest. Otherwise, this is in a branch called keysToLowercase if you want to take a peek. |
Ah, thanks Javan! That's great. I'm fine changing just the Date key for the time being to keep this within the original scope. At a glance the function looks handy, very cool. Let me know when you have a PR up for it and I'll give it a run my side. |
Javan has added lower casing for the date issue, other keys are uppercase. This isn't a significant issue currently, but is something to be noted that is a quirk of the code as it must be accounted for which should be kept an eye on. |
Currently, the returns from a couple of the API's we are consuming uses a proper case convention. Other ones use all lower case. One even uses both.
This proves tricky when using one component across many datasets, as we do for the graphs. Currently a quick fix is in place for the graphs where we pass a property called dateCap (a boolean to indicate if the date key is capitalized or not) to the graph, and utilize that to access the data.
It would be better to instead normalize the data upon consumption, and re-case all object keys to lower case. We currently have Lodash as a dependency which could make this a little simpler.
Once they are all recased, then the workaround dateCap logic could be removed.
The text was updated successfully, but these errors were encountered: