-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improve advisor availability filter and general page loading user experience #2109
Conversation
Commits missing Jira IDs: |
cypress tests🗡️🗡️🗡️ |
@RedHatInsights/team-interact this PR is ready for review. PTAL! |
}, [cves?.meta?.cves_without_errata, isLoading]); | ||
|
||
setShowCvesWithoutErrata(includesCvesWithoutErrata); | ||
}, [includesCvesWithoutErrata]); |
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.
There should be more dependencies here. We should at least also add showCvesWithoutErrata
.
import { Bullseye } from '@patternfly/react-core'; | ||
import { Spinner } from '@patternfly/react-core'; | ||
|
||
export default () => ( |
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.
This is not good practice as React will have to figure out some display name for the component. The shared fec aslant
configure is set to warn about this, unfortunately vulnerability-ui
is not (yet) using the shared config.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2109 +/- ##
==========================================
- Coverage 67.58% 66.65% -0.94%
==========================================
Files 131 128 -3
Lines 3437 3440 +3
Branches 1066 1068 +2
==========================================
- Hits 2323 2293 -30
- Misses 1114 1147 +33 ☔ View full report in Codecov by Sentry. |
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.
Works as expected.
Having one spinner is a small change, but made a huge difference. Well done! 😄
Essentially, this is an effort to improve the perceived performance of CVEs page. Before this change, the whole table was triggered loading and once the request to fetch cves for the table resolves, according to the advisor_availability info in the metadata, a new filter 'Advisor availability filter' was added. This would result in complete re-render of the table and the same API request to be triggered once more. Now, the advisory availability info is fetched on the very first page load with limit:1 and provided across components using react context.
Furthermore, there is another improvement for the general page loading user experience. Before, for example, while CVEs page was loading, the spinner would be displayed first in the center, then on top smaller, then on top bigger. This was not very pretty. Now, as one component is shared, the spinner is displayed only in the center. This improvement needs to be tested with this fix in the fec-components package: RedHatInsights/frontend-components#2010