chore: avoid button 'flickering' during extension start #6863
Conversation
To stop the buttons from 'flickering' during start, switch the start button enablement to !started. This ensures that there is always one button visible (start or stop) so nothing 'moves'. One part of containers#6855. Signed-off-by: Tim deBoer <[email protected]>
deboer-tim
requested review from
dgolovin and
axel7083
and removed request for
a team
| @@ -16,7 +16,7 @@ async function startExtension(): Promise<void> { | |||
| } | |||
| </script> | |||
|
|
|||
| {#if extension.state === 'stopped'} | |||
| {#if extension.state !== 'started'} | |||
There was a problem hiding this comment.
it feels strange to see the start button if the state is not started.
let say state is 'unknown' or 'deleted' or something different we could see the button and this is not expected
probably need to tweak the accepted state including the starting or other intermediate steps
There was a problem hiding this comment.
The start/stop buttons already correctly enable/disable when they are not applicable, i.e. we could show both of them always and the page works fine. It is only our choice to only display one at a time to save space/reduce clutter, and flickering occurs because there are intermediate states where we have no button and so the layout adjusts.
This change is identical to what we did in the containers view, just making sure that the button visibility states are completely opposite so that you always & only see one button:
. Neither button is enabled in these states, i.e. we could do the opposite change to the stop button.There was a problem hiding this comment.
It is only our choice to only display one at a time to save space/reduce clutter,
to clarify, it's the person that did the mockup choice ;-)
and flickering occurs because there are intermediate states where we have no button and so the layout adjusts.
I understand the problem but I would avoid the usage of saying 'anything that is not started' to display the start button as while it does the trick currently, it does not guarantee it will always work as it can have side effects with the button 'stop' being displayed in some state that should not permit that.
so I would list there the states that are allowed (including the intermediate states) so it's clear that it will always be displayed for the states that we want/list and not something that may work but is fragile
There was a problem hiding this comment.
to clarify, it's the person that did the mockup choice ;-)
;-)
I understand the problem but I would avoid the usage of saying 'anything that is not started' to display the start button as while it does the trick currently, it does not guarantee it will always work as it can have side effects with the button 'stop' being displayed in some state that should not permit that.
so I would list there the states that are allowed (including the intermediate states) so it's clear that it will always be displayed for the states that we want/list and not something that may work but is fragile
We are missing tests for LoadingIconButton, but that is where we should really be verifying correct enablement of the buttons with different states. The Resources page has start/stop/restart all visible so that will help too.
Which means that the fragile part here is mostly just ensuring that if state === x and if state != x are exactly opposite between these two buttons and only one appears. In ContainerActions that's obvious enough because they're 9 lines apart, but not here. So, I've added tests to the lifecycle component that cover both which buttons appear in each state and enablement, let me know what you think.
There was a problem hiding this comment.
the condition is still erroneous to me. We can't hide a start button if we're in a state different than 'started'. This is semantically incorrect and it can lead to errors
If you prefer as you're telling that the LoadingIconButton is already smart. It's to add a property to this component so it'll hide if condition is not satisfied as well.
so instead of doing the if there, it's done inside the component that will control its display usage
Either it includes all the valid states where it should be displayed or it's inside the LoadingIconButton component as well but it should still use states semantically valid.
Add tests to ensure correct button visibility and enablement in each of the extension states. Signed-off-by: Tim deBoer <[email protected]>
might be related to containers#6863 Signed-off-by: Florent Benoit <[email protected]>
might be related to #6863 Signed-off-by: Florent Benoit <[email protected]>
What does this PR do?
Minor fix: to stop the extension buttons from 'flickering' during start, switch the start button enablement to !started. This ensures that there is always one button visible (start or stop) so nothing 'moves'.
Screenshot / video of UI
N/A, hard to catch flickering on video.
What issues does this PR fix or reference?
One part of #6855.
How to test this PR?
Stop and start an extension multiple times and ensure the buttons in that row don't move.