-
Notifications
You must be signed in to change notification settings - Fork 10
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
📅 Datepicker: set initial range, use unambiguous date format, use arrow
, dropdown for timezone
option
#96
Conversation
On the initial load, the user has not selected a date and we fallback to only showing the last week of data. The datepicker should convey this information rather than showing blank initially. This change sets initial start and end date values to the datepicker. As a result, we will no longer need extra checks for the initial condition when start date or end date have not been set yet. This simplifies the code in a few places.
Consider "1/5/2024" – in the US this seems like January 5, while in most of the world it seems like May 1. Whether we did D/M/Y or M/D/Y, it'd probably cause some confusion since the project is used on an international scale. So instead of just digits, the new format will be "5 Jan 2024". Unambiguous and still pretty concise.
This change adds the `arrow` package as a drop-in replacement for any places where `datetime` was used. The behavior should not change with this diff - this is only for code cleanliness & consistency with the server **Note: `arrow` does not have 'min' and 'max' variables like `datetime`. So, these are manually replaced. E.g. min time = 00:00, max time = 11:59, max timestamp = maximum epoch time
arrow
Switched to using We do not |
From e-mission#73 (comment) Implements a dropdown which displays below the datepicker and allows choosing between UTC vs local time as the basis for the date selection queries. So 'timezone' is passed now as an additional argument to the querying functions.
I noticed this code is repeated and since timezone is passed as a 3rd argument 'tz', it warrants extracting to a separate function Created a function 'get_ts_range' to use in both places.
As described in the code comment, this code is unused and we are not currently filtering UUIDs by datetime, so updated the code to make this clearer
arrow
arrow
, dropdown for timezone
option
Added a dropdown for UTC time vs. Local timezone (as described in #73 (comment)) UI now looks like this: |
This weird formatting was from vscode autoformatting (which I have now disabled for python projects) I know that Python is a language where whitespace / linebreaks can alter the function of the code so I want to be extra careful here
As it turns out, the datepicker does not keep current to "today's" date for the selection. The reason for this is to do with how Plotly Dash works – the DatePicker component gets generated once when the dashboard is deployed, and does not update on every page load. Our last production deployment was Jan 15, so that is what it is locked to. Described in https://community.plotly.com/t/refreshing-the-start-date-everyday-for-datepickersingle/22234 According to https://dash.plotly.com/live-updates ("Updates on Page Load"), if we define the page content as a function rather than a variable, it will be generated on each page load / refresh. |
For better organization
fixes the issue described in e-mission#96 (comment) To recompute a new date on refresh, "layout" needs to be a function. This required reorganizing the page contents. Some things can stay static but the things that need to be recomputed are reorganized into "make_" functions. Inside make_controls() is where we generate the dates, and this gets called on every refresh. The resulting DOM is the same.
Fixed in d9c7df2 To prove that it was actually updating and for the purpose of testing, I injected a simple Every time I refresh, the 'seconds' updates, which proves that the controls are being re-computed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other that the single refactor below.
app_sidebar_collapsible.py
Outdated
# trim the time part, leaving only date as YYYY-MM-DD | ||
start_date = start_date[:10] if start_date else None | ||
end_date = end_date[:10] if end_date else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is repeated multiple times and should be pulled out into a separate function.
This is not just for lowering the lines of code (from 2 to 1) but consolidating the calls so that if we ever change the display/parse method, we can do so in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be much better! I didn't see any existing files where a function like that would make sense to go in.
So to refactor, I think I will create a new file utils/datetime_utils.py
with this new function, and maybe also the get_ts_range
function (which I had created in db_utils
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored
This code was repeated many times and should be extracted to a separate function. Did it in such a way that it accepts any number of inputs, thanks to Python's *args syntax Put this in a new file since none of the other 'utils' files had anything to do with datetime
This function is used by db_utils but might be better categorized in datetime_utils. Previously called`get_ts_range`, its new name is `iso_range_to_ts_range`, since I thought that was more descriptive. -- Also, I noticed `arrow` was imported in db_utils twice, so i fixed that while I was here
set initial date range on datepicker
use unambiguous format on datepicker (D MMM Y)