fixes #1589: debounce and read snackbar for assignment planning #1623
Conversation
|
One usecase to test is the change of events like setting a Mini course goal, individual assignment goal, lock goal etc Is being logged in the backend with I did not test this, but that is the important usecase to test. |
| @@ -13,22 +14,27 @@ function UserSettingSnackbar (props) { | |||
| saved, | |||
| response, | |||
| successMessage = 'Setting saved successfully!', | |||
| failureMessage = 'Setting not saved.' | |||
| failureMessage = 'Setting not saved.', | |||
| debounceAmount = 0 | |||
There was a problem hiding this comment.
Can you add a comment the purpose of this variable?
There was a problem hiding this comment.
same question. Not sure why we need this debounceAmount param.
There was a problem hiding this comment.
I added a new comment to clarify in the new version of this file. There are other areas of the app that use this snackbar that we don't want to add a debounce to, at least for this issue. For instance, the grade filter setting for the resources accessed page triggers the snackbar. I've left it customizable for the future in case other pages would like a debounce
| @@ -32,7 +32,7 @@ function AssignmentGoalInput (props) { | |||
| } = props | |||
|
|
|||
| const [goalGradeInternal, setGoalGradeInternal] = useState(goalGrade) | |||
| const debouncedGoalGrade = useRef(debounce(q => setGoalGrade(q), 500)).current | |||
| const debouncedGoalGrade = useRef(debounce(q => setGoalGrade(q), 250)).current | |||
There was a problem hiding this comment.
Why was the number reduced?
There was a problem hiding this comment.
When I added a long debounce to the snackbar, the duration of this save would stack on top of the duration for the snackbar pop up. It felt quite long compared to other inputs on the page, so I reduced this debounce amount. I didn't consider if there was any drawback to lowering this, other than the visualizations and input amounts rendering slightly faster
There was a problem hiding this comment.
If it improve overall objective of the view that it is fine
| @@ -293,6 +293,7 @@ function AssignmentPlanningV2 (props) { | |||
| <UserSettingSnackbar | |||
| saved={!mutationError && !mutationLoading && settingChanged} | |||
| response={{ default: 'success' }} | |||
| debounceAmount={750} | |||
| /> | |||
There was a problem hiding this comment.
This is milli second i suppose? why is this number chosen?
There was a problem hiding this comment.
Not much thought went into it other than to add a buffer approximately around the duration of the other debounced input we had (originally 500ms). I ran it by @gsilver, and the 500-1000ms option seemed to be a good range. I'm also open to other guidance on this (and we can customize it later)
There was a problem hiding this comment.
I think 750 seems fine, I just want to understand the reason.
|
To Pushyami's comment "One usecase to test is the change of events like setting a Mini course goal, individual assignment goal, lock goal etc Is being logged in the backend with eventlog_logand only once. Since this change GraphiQL mutation API, so we want to make that is functional as well" I have verified the event log count is equal to the snack bar appearance. So if I updated the goal multiple times within the 750 ms, I see only one snack bar and only one new event log record. |
| @@ -13,22 +14,27 @@ function UserSettingSnackbar (props) { | |||
| saved, | |||
| response, | |||
| successMessage = 'Setting saved successfully!', | |||
| failureMessage = 'Setting not saved.' | |||
| failureMessage = 'Setting not saved.', | |||
| debounceAmount = 0 | |||
There was a problem hiding this comment.
same question. Not sure why we need this debounceAmount param.
Use the debounce package to delay opening the snackbar, also modify the snackbar component to alert screen readers instead of being hidden previously.