XiaoW Add tooltip to new timer and cleanup old timer #1455
Conversation
…yForm for better UX
There was a problem hiding this comment.
Hi @wang9hu, I have tested the PR locally and there seem to be some bugs while using the "Stop" button on the dashboard.
Firstly, the tooltips work fine and are visible when hovered over.
Secondly, when logging time before timer runs out, the next goal is what is remaining from last timer but to be mentioned when trying to stop the timer using the button on the dashboard it seems unresponsive and I could only stop the timer and log the time when the stop button was accessed via the "clock" icon dropdown (top-left).
Thirdly, Logging time after timer runs out, the next goal is what is set as the initial goal but when the Log Time popup appeared and I clicked on "Log Time", the actual Log Sheet didn't appear until again the "clock" icon was clicked. Refer to my recordings for better understanding and let me know when further testing needs to be done.
Thank you for your work.
Test rec:
Tooltips on hover:
26.10.2023_20.08.31_REC.mp4
Logging time before timer runs out, the next goal is what is remaining from last timer:
26.10.2023_20.09.21_REC.mp4
Logging time after timer runs out, the next goal is what is set as the initial goal:
26.10.2023_20.17.27_REC.mp4
|
@malikjahanzaib Thank you for your review! Nice catch! |
Hi @wang9hu, works perfectly. Bug Fixed. Approving! Test rec(s): 27.10.2023_17.44.31_REC.mp427.10.2023_18.29.22_REC.mp4 |
This reverts commit 7ee6235.
|
Hey, I have reviewed and tested the MR. It works as intended. The tooltips are working well and the goals before and after logging the time also work correctly. Great work. 1455.mp4 |
There was a problem hiding this comment.
Hi @wang9hu , I've tested your PR and everything that you said to test worked fine! When logging time after the timer ends the timer goal is set to the previous initial goal (see video on timestamp 01:46) and when logging before the timer ends the timer goal is set to the remaining time (see video below on timestamp 05:00).
On the video below I was trying to break the clock by stopping and resetting the timer rapidly and it didn't break.
Issues
- Tooltip not showing on pause button inside the dropdown (see video below on 3:50);
- Console reporting an error about the Redux reducer keys (I think it is, correct me if I'm wrong) as the screenshot below shows;
- (Suggestion) I think there should be a tooltip on the clock icon, saying that it opens the timer dropdown. As it is clickable but doesn't show any tooltip;
- (Suggestion) Also on the tooltips, I think it would be interesting to have a tooltip on the little hourglass saying that it is registering your time or something like that. Just an idea that I had :).
Video and Screenshot
video1817902655.mp4
Final thoughts
Overall, great job with this PR! It worked as intended and when the changes are made I'll be reviewing it again! 👍
|
Hey @wang9hu, initial.movTime.elapsed.and.break.movYour did a great job! |
There was a problem hiding this comment.
It's an improvement over the current one on the dev. I'm no longer able to submit duplicate time entries by hitting submit multiple times.
issues I'm still experiencing are that there's a delay in closing the modal after hitting submit.
Screen.Recording.2023-10-28.at.4.53.31.PM.mov
Also experiencing the same issue about the reducer keys in the console.
There was a problem hiding this comment.
Reviewed this PR and these things work:
1.The tooltips look great.
2.When logging time before timer runs out, the next goal is what is remaining from last timer
I had the following issue:
- I had trouble submitting a time entry. The page would not respond after I clicked submit button, I then clicked it several times until it responded and received several duplicate time records as a result. See video:
submitting.time.entries.several.times.mov
And I have one question:
After submitting one time entry with time remaining from the total goal I start the timer again to complete the goal. When I click the reset button, the timer is reset to the original goal. Wouldn't it make more sense if it was reset to the time remaining? Because a user has already logged some time so there is no need to reset it to the original goal. Not a big deal, but just some thoughts...
Here is a video of resetting:
tooltips.and.resetting.mov
|
Thank you all for your prompt reviews!
@savio-henrique That might be cleared with a hard refresh, try that and let me know if the issue is still there. Your other suggestions are also great!
@KurtisIvey Unfortunately That is how the TimeEntryForm works now, and it takes long time to submit. I personally believe that also needs a complete overhaul, but I think it is beyond the purpose of this new timer implementation. I will take that as my next task after this. And the reducer key issue please try hard refresh, let me know if that it persists.
@cvtqx I made some changes to the TimeEntryForm so that now user can not spam the button, and the button will show 'Submitting' as a little improvement of UX. Thanks for pointing this out, nice catch!
@cvtqx Thanks for the advice! After discussion with Jae, I think we will just stick to what we have for now, since the timer already auto set its goal to the remaining time, and if the user want to change its current remaining time, just use the add/subtract buttons to do so, no need to reset to its previous remaining time. The reset should just for resetting to its initial goal. |
…https://github.com/OneCommunityGlobal/HighestGoodNetworkApp into xiaoW_add_tooltip_to_new_timer_and_cleanup_old_timer
There was a problem hiding this comment.
Hello, @wang9hu! Great implementation! I tested your changes and they are quite the improvement from the previous release of the timer.
The timer buttons have tooltips displayed on hover and when logging the time before the timer runs out, the next goal is set to the time remaining from the last timer:
before_time_runs_out.mp4
When logging the time after the timer runs out, the next goal is what is set as the initial goal:
after_time_runs_out.mp4
If there are multiple tabs, the ringing effect is consistent:
multiple_tabs.mp4
And, the bug that I reported on Slack where the color and background color of '-15 m' button were faulty when the time is maximum, is also fixed:
Before:
Now:
Description
This PR cleans up the old timer related code and files, and adds tooltip to buttons on the new timer. Also included goal changes for the timer behavior adjustment, details are in PR#586
Related PRS (if any):
This frontend PR is related to the backend PR#586
Main changes explained:
How to test:
npm installandnpm run start:localto run this PR locallyNote:
Try clear cache and hard refresh if you see a key error from reducer. Let me know if that error persist after hard refresh.