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.

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

KOGITO-1299: added fallback UI when server unavailable #131

Merged
merged 1 commit into from
Apr 6, 2020

Conversation

Sara4994
Copy link
Contributor

please enter the commit message for your changes. Lines starting

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 3 times, most recently from 7e7d017 to 8457ccc Compare March 30, 2020 12:10
@Sara4994
Copy link
Contributor Author

Sara4994 commented Mar 30, 2020

Hey @srambach @kywalker-rh,

Have developed pages to display 2 different things in management console,

  1. A fallback UI when the server is not available/down, and here is the screenshot of the UI.
    Screenshot from 2020-03-30 17-53-19

  2. UI to display when there are some error occurs on the server and to display that error in the front-end. Screenshot follows,
    Screenshot from 2020-03-30 17-47-01

It is just developed temporarily for now, awaiting for your comments to modify and to make it more relevant. Please have a look when you get time.

@srambach
Copy link
Contributor

@Sara4994 nice job! I went over them with @kywalker-rh.
#1, the server error seems good as is.

#2, the graphql error - we were thinking of the following changes:
first line of text: An error occurred while accessing data. See more details. and the More details would have a popover containing the error text. If need be, we can use the maxWidth property on the popover component to adjust it so that it doesn't stay narrow and stretch very tall with a long error message.
button: it doesn't seem that "refresh" would help in this case, so perhaps Go back ?

@cristianonicolai wdyt?

@cristianonicolai
Copy link
Contributor

@srambach +1

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 2 times, most recently from dbae9c6 to 60fccf0 Compare March 31, 2020 10:42
@Sara4994
Copy link
Contributor Author

@Sara4994 nice job! I went over them with @kywalker-rh.
#1, the server error seems good as is.

#2, the graphql error - we were thinking of the following changes:
first line of text: An error occurred while accessing data. See more details. and the More details would have a popover containing the error text. If need be, we can use the maxWidth property on the popover component to adjust it so that it doesn't stay narrow and stretch very tall with a long error message.
button: it doesn't seem that "refresh" would help in this case, so perhaps Go back ?

@cristianonicolai wdyt?

@srambach corrections are made..
Screenshot from 2020-03-31 17-02-18

color="var(--pf-global--danger-color--100)" />
<Title headingLevel="h1" size="4xl">Error fetching data</Title>
<EmptyStateBody>
Below error occured on graphql while accessing data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change this text to An error occurred while accessing data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srambach done

@@ -526,7 +526,7 @@ const DataListItemComponent: React.FC<IOwnProps> = ({
<DataListItemCells
dataListCells={[
<DataListCell key={1}>
<Link to={'/ProcessInstances/' + processInstanceData.id}>
<Link to={{pathname:'/ProcessInstances/Process/' + processInstanceData.id, state:{ prev: location.pathname}}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 would it be something like:

Suggested change
<Link to={{pathname:'/ProcessInstances/Process/' + processInstanceData.id, state:{ prev: location.pathname}}}>
<Link to={{pathname:'/Process/' + processInstanceData.id, state:{ prev: location.pathname}}}>

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmirasch
Copy link
Contributor

nmirasch commented Apr 1, 2020

@Sara4994 (+1 to add the new path at VertxRouter) and it seems some .snap files have to be also updated with the new path.

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 4 times, most recently from 08b4d0c to a264bb7 Compare April 1, 2020 10:54
@Sara4994
Copy link
Contributor Author

Sara4994 commented Apr 1, 2020

@Sara4994 (+1 to add the new path at VertxRouter) and it seems some .snap files have to be also updated with the new path.

@nmirasch done

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 3 times, most recently from d6e0a5d to 85a76a5 Compare April 2, 2020 15:32
Copy link
Contributor

@bxf12315 bxf12315 left a comment

Choose a reason for hiding this comment

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

The PR can fix issue KOGITO-1659.

@@ -18,8 +18,6 @@ const NoDataComponent = (props) => {
let prevPath;
if (props.location.state !== undefined) {
prevPath = props.location.state.prev;
} else {
prevPath = '/ProcessInstances'
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 shouldn't this stay for when users hit url directly without a previous state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cristianonicolai yep removed by mistake, added it back. thanks

@@ -41,6 +41,7 @@ public void init() {
void setupRouter(@Observes Router router) {
router.route("/").handler(ctx -> ctx.response().putHeader("location", "/ProcessInstances/").setStatusCode(302).end());
router.route("/ProcessInstances*").handler(ctx -> handle(ctx));
router.route("/Process*").handler(ctx -> handle(ctx));
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 The new route "/Process*" includes "/ProcessInstances*. Here I think we have to remove the line: router.route("/ProcessInstances*").handler(ctx -> handle(ctx)); and let just the new route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmirasch but actually these two are different routes that directs us to two different pages, '/ProcessInstances' will take us to process list page and '/Process' will take us to process details page. should we still remove the line: router.route("/ProcessInstances*").handler(ctx -> handle(ctx));?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 point is that /Process* will also match the same urls, because of the pattern matching, so the line with /ProcessInstances* should be removed or you can simply update that one.

@@ -1,15 +1,36 @@
import '@patternfly/patternfly/patternfly.css';
import React from 'react';
import ReactDOM from 'react-dom';
import ApolloClient from 'apollo-boost';
import { ApolloClient } from 'apollo-client';
// import ApolloClient from 'apollo-boost';
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 maybe remove the comment :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nmirasch this is done :)

An error occured while accessing data. <strong className="kogito-management-console--Server-Errors__text-color" onClick={() => setDisplayError(!displayError)}> See more details</strong>
</EmptyStateBody>
{displayError &&<EmptyStateBody>
<ClipboardCopy isCode variant={ClipboardCopyVariant.expansion} isExpanded={true}>{JSON.stringify(props.message)}</ClipboardCopy>
Copy link
Contributor

Choose a reason for hiding this comment

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

We can left align the text and make it more readable by adding className="pf-u-text-align-left" to the ClipboardCopy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srambach done:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, one more thing - I missed that "occurred" is still spelled incorrectly. 2 rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srambach done )

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 3 times, most recently from f23e3fe to a97d9cd Compare April 3, 2020 14:47
color="var(--pf-global--danger-color--100)" />
<Title headingLevel="h1" size="4xl">Error fetching data</Title>
<EmptyStateBody>
An error occurred while accessing data. <strong className="kogito-management-console--Server-Errors__text-color" onClick={() => setDisplayError(!displayError)}> See more details</strong>
Copy link
Contributor

@srambach srambach Apr 3, 2020

Choose a reason for hiding this comment

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

The "See more details" needs to be coded as a link or button. Otherwise, it is not accessible using the keyboard. <Button variant="link" isInline onClick={() => setDisplayError(!displayError)}>See more details</Button> should work for you I think. We don't need the <strong> tag and don't need to set the text color or cursor with a new class since that will come from the link. So that CSS can also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srambach done. Thanks 👍

@@ -122,7 +124,8 @@ const DomainExplorerColumnPicker: React.FC<IOwnProps> = ({
}
});
} catch (error) {
return error;
setError(error)
// return error;
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sara4994 remove comment out code please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

</PageSection>
</>):(<ServerErrorsComponent message={error.message} />)}
</>);
{/* {!error ?
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove comment out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sara4994 Sara4994 force-pushed the Kogito-1299 branch 2 times, most recently from bd035c2 to d47c00c Compare April 6, 2020 06:24
@sonarcloud
Copy link

sonarcloud bot commented Apr 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@cristianonicolai cristianonicolai merged commit 8a7e2bf into apache:master Apr 6, 2020
almope pushed a commit to almope/kogito-apps that referenced this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants