Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Warning banner improvement #1371

Merged
merged 3 commits into from
May 2, 2023
Merged

Warning banner improvement #1371

merged 3 commits into from
May 2, 2023

Conversation

nelsonkopliku
Copy link
Member

@nelsonkopliku nelsonkopliku commented Apr 28, 2023 •
edited
Loading

Description

Follows up #1244 by addressing the action point mentioned in #1253 (comment).

The warning banners can be specific for the checks results.

How was this tested?

Added tests for banner retrieving and adjusted relevant tests.

@nelsonkopliku nelsonkopliku self-assigned this May 2, 2023
@nelsonkopliku nelsonkopliku added chore ux javascript Pull requests that update Javascript code labels May 2, 2023
@nelsonkopliku nelsonkopliku marked this pull request as ready for review May 2, 2023 07:44
@nelsonkopliku nelsonkopliku mentioned this pull request May 2, 2023
Merged
EMaksy
EMaksy reviewed May 2, 2023
View reviewed changes
Copy link
Member

@EMaksy EMaksy left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you thing of using the conditional operator ? ?
Something like this:
export const getProviderWarningBanner = (provider) => providerWarningBanners[provider]?.default || null;
Or what do you think ?

EMaksy
EMaksy reviewed May 2, 2023
View reviewed changes
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey nice work, besides the comment suggestions LGTM :D

assets/js/components/Banners/WarningBanner.jsx Outdated Show resolved Hide resolved
dottorblaster
dottorblaster approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nelsonkopliku
Copy link
Member Author

nelsonkopliku commented May 2, 2023 •
edited
Loading

@dottorblaster in the last commit 7e8ee13 I applied your suggestion about getting rid of providerWarningBanners and related banner retrieval functions.
Scattering around the same contents conflicts with my OCD, but still, if you prefer like that I don't mind 😄

@EMaksy take a look at this please as it changes the base for your comments.

EMaksy
EMaksy approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey nice work, LGTM :D

@nelsonkopliku nelsonkopliku merged commit 430f29c into new-checks-ui May 2, 2023
@nelsonkopliku nelsonkopliku deleted the warning-banner-improvement branch May 2, 2023 13:29
nelsonkopliku added a commit that referenced this pull request May 4, 2023
@nelsonkopliku
Warning banner improvement (#1371)
c74bcfa 
* Refactor providerWarningBanners to also support result specific banners and expose retrieving functions

* Use the banner retrieving functions

* Refactor warning banners usage
nelsonkopliku added a commit that referenced this pull request May 4, 2023
@nelsonkopliku
Warning banner improvement (#1371)
cfa8fcc 
* Refactor providerWarningBanners to also support result specific banners and expose retrieving functions

* Use the banner retrieving functions

* Refactor warning banners usage
nelsonkopliku added a commit that referenced this pull request May 4, 2023
@nelsonkopliku @dottorblaster
@EMaksy @arbulu89
New checks UI (#1253)
0d11b0c 
* Check based execution results (#1223)

* Add an ExecutionHeader component

* Add a selector to get hostnames of a given cluster

* Refactor ExecutionResults to a Check based visualization

* Object tree component (#1228)

* Fix storybook configuration with @static alias

* Add object tree factory

* Add ObjectTree React component

* Add tests

* Add react-accessible-treeview and a webpack plugin required for
Storybook

* Add desctructuredArrayIgnorePattern option to eslint

* Address review comments

* Add WIP

* Re-add checks filtering (#1231)

* Remove debug code (#1235)

* Improve error handling on checks results page (#1232)

* Enhance NotificationBox to support straight message text or a list of messages to show

* Improve error handling on checks results page

* Keep a loading box while the execution is running

* Fix ExecutionHeader test with new warning text

* Open remediation modal when check id is clicked in the ExecutionResults view (#1256)

* Open remediation modal when checkId is clicked

* Fix determining expectations results (#1246)

* Add a getCheckExpectations utility to support check based view

* Improve CheckResultOutline to properly handle more scenarios

* Adjust ExecutionResults component so that it provides catalog expectations to a Check result outline

* Address review comments

* Adjust styling

* Check result detail pt1 (#1248)

* Chore: Use getClusterHostNames in ExecutionResultsPage

* Make TargetResult clickable

* Navigate to CheckDetail page when clicking on a target result row

* Add an ExecutionContainer component that wraps loading states

* Add a CheckResultInfoBox component that gives information about a check's execution context

* Add a CheckDetailHeader that provides the introductory section of a check detail page

* Add an initial version of the CheckResultDetail

* Add a CheckResultDetailPage as a container interacting with the state

* Wire up the newly created CheckResultDetailPage to the app router

* Improve component testing

* Improve selection for ExecutionResultsPage and CheckResultDetailPage

* Rename getLastExecutionContext selector to getLastExecutionData

* Improve loading effects

* Improve cluster hosts retrieval for execution results/check detail (#1268)

* Chore: cleanup CheckResultInfoBox classname

* Add a getClusterHosts to get all the hosts of a cluster

* Use getClusterHosts in getLastExecutionData selector

* Adjust test factories in relation to hosts

* Refactor ExecutionResults to use the provided cluster hosts

* Refactor CheckResultDetailPage to use provided cluster hosts and pass those to the check detail

* Add empty state subcomponent for the table component (#1279)

* Add empty state component and test

* Check Result Detail pt2 (#1264)

* Chore: extract and test transformation functions from CheckResultOutline

* Implement a Check detail for a Host target

* Determine the target id based on target type in CheckResultDetailPage

* Split CheckResultDetail subcomponents in dedicated files

* Address review feedbacks

* Improve naming

* Visually identify premium checks in the ExecutionResults view (#1293)

* Add PremiumPill to ExecutionResults component

* Cluster wide Check Result Detail (#1291)

* Add an HealthIcon to CheckDetailHeader

* Centralize enrichment of execution data agents check results with hostnames into selector

* Cleanup extraction of expect_same statements results

* Support both host and cluster wide ExpectationsResults

* Add the ability to extract facts for cluster wide checks

* Add support for both host and cluster wide facts to GatheredFacts component

* Make CheckResultDetail support host and cluster wide checks

* Address review feedbacks

* Fact value primitive types (#1317)

* Display raw values for primitive types

* Add FactValue tests and detail stories

* Fix factories using transient params

* Fix props in CheckResultDetail test

* Add some margin between cluster-wide facts (#1336)

* Clean up legacy code (#1325)

* Catalog provider filter (#1363)

* Move provider filter to the right in catalog view

* Add ProviderLabel storybook story

* Update providerSelection and add story

* Use the update ProviderSelection in catalog

* Handle All entry in provider selection

* Remove unwanted comments

* Use all providers in ProviderSelection story

* Refacto buildUpdateCatalogAction function

* Add url error handling for the CheckResultDetailPage (#1364)

* Refactor NotFound component, add test and story

* Add url error handling for CheckResultDetailPage

* Collapsible checks catalog (#1369)

* Add an Accordion Component

* Refactor ChecksCatalog to have collapsible groups

* Adjust cypress test to select correct item in the DOM

* Refactor CheckItem to use the new Accordion component

* Refactor ChecksSelectionGroup to use the new Accordion component

* Adjust z-index for ProviderSelection

* Warning banner improvement (#1371)

* Refactor providerWarningBanners to also support result specific banners and expose retrieving functions

* Use the banner retrieving functions

* Refactor warning banners usage

---------

Co-authored-by: Alessio Biancalana <alessio.biancalana@suse.com>
Co-authored-by: EMaksy <emaksymenko@suse.com>
Co-authored-by: Xabier Arbulu Insausti <xarbulu@suse.com>
@nelsonkopliku nelsonkopliku mentioned this pull request Sep 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EMaksy EMaksy EMaksy approved these changes

@dottorblaster dottorblaster dottorblaster approved these changes

@arbulu89 arbulu89 Awaiting requested review from arbulu89

Assignees

@nelsonkopliku nelsonkopliku

Labels
chore javascript Pull requests that update Javascript code ux
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nelsonkopliku @dottorblaster @EMaksy