Closed lorikku closed 5 months ago
@tomcru writing a test necessary here? Would require to set up React component testing, also dependency injection to get holyLoader.status
and check if it equals (1) when it should etc.
Otherwise ready to be reviewed and merged!
This looks exciting and I've had something similar in mind - will test it out asap!
This looks exciting and I've had something similar in mind - will test it out asap!
So apparently it's the good ol' "it used to work but now it doesn't" scenario.. I just tested rn and it's not working anymore, will have to see what's wrong and update it here.
Bugs to fix:
I'll get it sorted out soon, for now will put this in draft.
Okay so I realised that this won't ever work on the server side because I use document.dispatchEvent
which requires document
which is not available on the server.
Anything requiring UI interactivity will forever be restricted to Client components only (which makes sense obv). That means that this functionality will too.
I marked it in the documentation to make sure people know. But it definitely works now on client side now!
@tomcru ready to review now!
I was thinking the same, that'd you'd only ever use the trigger funcs to start/stop the loader in client components anyways 👍
Will review either today or tomorrow latest, thank you!
I noticed a little edge case where you can theoretically dispatch a ton of events which messes with the behaviour of the progress bar (I'm frantically clicking the Start button in this video) https://github.com/tomcru/holy-loader/assets/35841182/de825998-45f6-420f-9fa7-52fba7ac01c7
Compared to the normal behaviour: https://github.com/tomcru/holy-loader/assets/35841182/bbc9fa8e-3ffa-4520-aa4f-b6711837f067
I noticed a little edge case where you can theoretically dispatch a ton of events which messes with the behaviour of the progress bar (I'm frantically clicking the Start button in this video) https://github.com/tomcru/holy-loader/assets/35841182/de825998-45f6-420f-9fa7-52fba7ac01c7
Compared to the normal behaviour: https://github.com/tomcru/holy-loader/assets/35841182/bbc9fa8e-3ffa-4520-aa4f-b6711837f067
I think this might to be an issue related to the startTrickle
function, which creates a setTimeout
every time it's fired, without checking if any other is in progress already. I'll try to fix it now.
@tomcru I fixed the issue by adding a incrTimeout
variable which gets cleared when startTrickle
is called.
Before:
After:
Maybe a simpler solution would be to update the HolyProgress
start()
method from:
public start = (): HolyProgress => {
if (this.status === null) {
this.setTo(0);
}
this.startTrickle();
if (this.settings.showSpinner === true) {
this.createSpinner();
}
return this;
};
to:
public start = (): HolyProgress => {
if (this.status === null) {
this.setTo(0);
this.startTrickle();
if (this.settings.showSpinner === true) {
this.createSpinner();
}
}
return this;
};
As a result, incrementing is only started if it's a new progress bar - and not if it is already running.
This works on my side, can you possibly confirm this also works in your use case?
@tomcru works like a charm! Pushed the solution right now as well :)
Thank you! good stuff 🔥
@tomcru been testing this yesterday throughout my platform, it works insanely well. Very happy with the results and good UX addition.
Description
I've had a few instances where I needed to trigger the holy loader manually (eg. because of an async operation), but I couldn't because the possibility wasn't there. So I added two new functions (
startHolyProgress
andstopHolyProgress
) to control the holy loader manually. Simple yet very useful.Concretely what was done:
React.useRef
because usinglet
is not best practice. It's important to keep the same reference to the HolyLoader object, using alet
does not guarantee this. Aref
is the correct method. (let
actually caused a bug when I was trying tostopProgress
which would fire butholyLoader.complete()
didn't do anything!)startHolyProgress
andstopHolyProgress
to control the holy loader manually using custom events, meaning you can only use these in Client components!sppeed
->speed
)Type of change
Please delete options that are not relevant.
React.useRef
usageChecklist: