-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Client Count Calendar widget updates #13777
Conversation
@@ -42,7 +43,6 @@ class CalendarWidget extends Component { | |||
@tracked disablePastYear = this.isObsoleteYear(); // if obsolete year, disable left chevron | |||
@tracked disableFutureYear = this.isCurrentYear(); // if current year, disable right chevron | |||
@tracked showCalendar = false; | |||
@tracked showSingleMonth = false; |
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.
removed from calendar widget. This was a dropdown option we are no longer going to show in 1.10
@@ -34,18 +34,6 @@ | |||
</div> | |||
</button> | |||
</li> | |||
<li class="action"> |
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.
Again, removing all "Single Month" code that we had previously displayed in the calendar-widget dropdown.
this.toggleShowCalendar(); | ||
this.args.handleClientActivityQuery(month, year, 'endTime'); | ||
} | ||
|
||
@action |
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.
All things singleMonth were removed because this is not something we're now doing in 1.10
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.
Wow nice work figuring that time stuff. I think in general with all the reformatting of data in the components, we're going to have a better time in the future AND a more stable UI experience if we pull out as much of that into util functions as possible (either in a new file or just exported from the component), so that we can unit test them. Definitely does not have to be done for this PR, but something to consider as we get into testing
ui/app/adapters/clients/activity.js
Outdated
let utcDate = this.utcDate( | ||
new Date(Number(start_time.split(',')[1]), Number(start_time.split(',')[0] - 1)) | ||
); | ||
start_time = utcDate.toISOString(); |
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 is formatting it as ISO 8601 but above we call out RFC3339 -- We should format as RFC3339 for stability
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.
also by using formatRFC3339(new Date(2021, 9))
we can use the 0-indexed month
ui/app/adapters/clients/activity.js
Outdated
}, | ||
// called from components | ||
queryClientActivity(start_time, end_time) { | ||
checkTimeType(query) { |
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.
nit: this method is so long that I think a name change here would be helpful to figure out what's going on. Something like formatTimeParams
is indicative of the type of response one should expect, and what's happening within the code.
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.
Good idea and good name. Amended.
let endTimeFromLicense = `${activity.endTime.split('-')[1].replace(/^0+/, '')},${ | ||
activity.endTime.split('-')[0] | ||
}`; | ||
let startTimeFromLicense = `${license.startTime.split('-')[1].replace(/^0+/, '')},${ |
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 line and the one above are very similar, it might be worth splitting into another method. Then It might be easier to define what you expect from the license and what you expect to get back, and that makes it easier to test too!
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.
We refactored this, should look a little more DRY now.
} | ||
|
||
get isDateRange() { | ||
return !isSameMonth(new Date(this.startTime), new Date(this.endTime)); | ||
return !isSameMonth( |
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.
Just a note - this getter will change when my history/csv PR is merged
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.
👏 nice work! A couple minor things and then I think it's good to go
@@ -39,21 +39,21 @@ export default class Dashboard extends Component { | |||
@tracked isEditStartMonthOpen = false; | |||
@tracked responseRangeDiffMessage = null; | |||
@tracked startTimeRequested = null; | |||
@tracked startTimeFromResponse = this.args.model.startTimeFromLicense; // ex: "3,2021" | |||
@tracked startTimeFromResponse = this.args.model.startTimeFromLicense; // ex: ['2021', 3] is April 2021 (0 indexed) |
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.
great comments, thank you for the example!
this.endTimeRequested = null; | ||
} | ||
// clicked "Custom End Month" from the calendar-widget | ||
if (dateType === 'endTime') { | ||
// use the currently selected startTime for your startTimeRequested. | ||
this.startTimeRequested = this.startTimeFromResponse; | ||
this.endTimeRequested = `${month},${year}`; // "1, 2021" | ||
this.endTimeRequested = [year.toString(), month]; |
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.
month
here is a string if I'm reading correctly, do you mean monthIndex
?
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.
endTime and startTime come in a little different. One comes in as a month name (startTime) whereas endTime comes in as an index because it's a calendar of months instead of a list. I'll add a comment in the code.
This PR handles various Client Count startTime and endTime issues.
?end_time=2020-06-30T00%3A00%3A00Z&start_time=2020-06-01T00%3A00%3A00Z
, with nothing after the Z indicating UTC/zulu time). This is done using thedate-fns-tz
.Here is a stack overflow article on the issue, which has a complicated solution that was solved with the library added here.
formattedStartTime
andformattedEndTime
that converts the API returned RFC3335 timestamp into a Month,Year string. This is then consumed by the Dashboard.js component, which updates the tracked propertiesstartTimeFromResponse
andendTimeFromResponse
that ultimately display the dates on the UI as April 2019, etc.Here is a video recording show all the different flows as well as the network request to prove the data is being requested properly. Note the calendar widget is offset only because of the console window being open, it works as expect if that is not open.
Screen.Recording.2022-01-31.at.11.08.57.AM.mp4