-
Notifications
You must be signed in to change notification settings - Fork 15
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
Extend error handling of AvailableSoftwareUpdates
component
#2538
Conversation
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.
Hey @jamie-suse, I guess there is something we need to address:
In a host detail page, even if there are settings, the Settings
CTA is shown
How to reproduce:
- configure suma settings
- browse to any host detail (you should see what expected)
- refresh host detail page (you should see the CTA even though settings are available)
This is happening because we are selecting from a piece of state that gets populated only if we browse settings page.
If we want to rely on that piece of state we might want to load suma settings at application startup (I initially understood you would have relied on the result from /api/v1/hosts/:host_id/software_updates
, but I actually prefer the other approach)
@jamie-suse for the loading state can we remove the word, "Loading..." and just keep the preloading spinner. I think the space is small and the preloader should be enough to convey the loading state. Please see the attachment for reference, |
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.
Some changes to do but the overall design looks good, keep it up!
assets/js/common/AvailableSoftwareUpdates/AvailableSoftwareUpdates.jsx
Outdated
Show resolved
Hide resolved
assets/js/common/AvailableSoftwareUpdates/AvailableSoftwareUpdates.jsx
Outdated
Show resolved
Hide resolved
assets/js/common/AvailableSoftwareUpdates/AvailableSoftwareUpdates.jsx
Outdated
Show resolved
Hide resolved
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.
Apart from what @nelsonkopliku mentioned, everything fine 👍
639c93b
to
d1075c0
Compare
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.
Definitely a step forward, thanks @jamie-suse!
Back on the very initial feedback shared offline: I guess we're missing a loading state for the whole Available software updates section.
The API that tells us whether we have settings or not (/api/v1/settings/suma_credentials
) might take a while to load and the ux transition feels a bit cumbersome: we show by default the section with the CTA which might get suddenly replaced by indicators.
In the attached gif I faked a 5s delay in the controller to give a sense of what I mean.
Besides this the endpoint we rely on might even fail, but I believe we can safely fallback to the no-settings-available state in order to not add a failure option.
@jagabomb what do you think?
Besides this a tiny visual thing: the section with the CTA (when settings are not available) seems to need a bit of padding/margin adjustment
Thanks for this @nelsonkopliku and @jamie-suse. I still wonder why we show the Available Software Updates (No Settings state) by default if a user has saved SUMA settings? Would it not be better to start with a loading state (refer to image below) just showing the Available Patches and Upgradable Packages sections as the default state, if we know you have saved SUMA settings. Basically switching between the two states above can be misleading to the user since they may believe the settings had not saved correctly. |
8222bf2
to
0dcbcf0
Compare
@jamie-suse As per our huddle review, can we please change the loading state for the Patches and Packages section to text saying just "Loading..." with not pre loader icon. Rationale is that the multiple pre loaders seems to be visually confusing. |
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.
LGTM
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.
LGTM
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.
LGTM!
Description
This PR extends error handling for the
AvailableSoftwareUpdates
component.On initial load, the Host Details page will display a loading spinner while the software updates settings are being retrieved.
If once the response is received, no SUSE Manager settings are saved, display the following to the user:
If some SUSE Manager settings have been saved, we enter the second loading state:
If we get any error response from SUSE Manager, we display it to the user as follows:
Finally, there is a catch-all unknown state. Where if for some reason we receive a
2XX
response but there is no data in the response, we display the following:How was this tested?
Added unit tests for the new states