-
Notifications
You must be signed in to change notification settings - Fork 24
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
PSP-9900 : PIMS service availability notifications #4674
base: dev
Are you sure you want to change the base?
Conversation
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4674 |
</label> | ||
{systemChecks.length > 1 && ( | ||
<LinkButton | ||
data-testid="healtcheck-full-list" |
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.
nit: healthcheck-full-list
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.
updated
totalDuration: Date; | ||
// Dictionary of health information. | ||
entries: { | ||
PmbcExternalApi: { |
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.
each of these objects:
{
data: object;
duration: Date;
status: string;
tags: string[];
}
appears the same, could these be defined by an interface to reduce repetition?
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.
updated
return ( | ||
const [systemChecked, setSystemChecked] = useState<boolean>(null); | ||
const [systemDegraded, setSystemDegraded] = useState<boolean>(false); | ||
const [healthCheckIssues, sethealthCheckIssues] = useState<IHealthCheckIssue[]>(null); |
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.
nit: setHealthCheckIssues.
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.
updated
return systemDegraded && systemChecked ? ( | ||
<> | ||
<LoadingBar style={{ zIndex: 9999, backgroundColor: '#fcba19', height: '.3rem' }} /> | ||
<Styled.AppGridContainerWithHealth className="App" {...rest}> |
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.
How would you feel about using a css class to show/hide the health container? it seems like you are having to duplicate lines 109 to 115 because of this approach.
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.
I am not opposed to the idea, but managing the grid-template-areas without breaking the app layout is complicated enough.
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.
@eddherrera I tested this out, and this solution isn't viable as it causes the map to be de-rendered and re-rendered. Please change this so that the banner is added and removed as needed without re-rendering the entire application.
At the moment, it does not appear that this AC is satisfied: Each of these messages should be alterable via configuration on the frontend. Therefore, we should use keys to refer the services to the messages. |
updated tenant file to store messages. |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4674 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4674 |
import * as Styled from './styles'; | ||
|
||
const PublicLayout: React.FC<React.PropsWithChildren<React.HTMLAttributes<HTMLElement>>> = ({ | ||
children, | ||
...rest | ||
}) => { | ||
return ( | ||
const [systemChecked, setSystemChecked] = useState<boolean>(null); |
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.
I don't think this logic should be added to the publicLayout. The PublicLayout should be stateless, and just accept params for these health check items.
const { setModalContent, setDisplayModal } = useModalContext(); | ||
|
||
return ( | ||
systemChecks.length && ( |
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 appears to throw an NPE, as this is initialized with null until the promise returns.
No description provided.