change project details to render the layout before loading the resour… #6454
Conversation
…ce, create new layout and layout header for investments
Richard-Pentecost
changed the base branch from
main
to
feature-branch/TET-613-RefactorInvestmentProject
Passing run #50436 ↗︎
Details:
Review all test suite changes for PR #6454 ↗︎ |
|||||||||||||||
Richard-Pentecost
merged commit 5c8532d
into
feature-branch/TET-613-RefactorInvestmentProject
| const renderChildren = (project) => { | ||
| return Children.map(children, (child) => { | ||
| return cloneElement(child, { | ||
| investment: project, | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I would avoid this mechanism as it makes components that are dependent on each other appear to be independent. In this particular case, <InvestmentProjectLocalHeader /> doesn't seem to be dependent to <InvestmentLayout/> in any way, but in fact it depends on <InvestmentLayout/> to inject the investment property to <InvestmentProjectLocalHeader />.
<InvestmentLayout
// ...
localHeaderChildren={<InvestmentProjectLocalHeader />}
>
{/* ... */}
</InvestmentLayout>One day anothe developer might want to tweak the markup (for whatever reasons) and wrap the <InvestmentProjectLocalHeader /> in a <div/> and they would have a hard time figuring out why do they get a "TypeError: Cannot read properties of undefined (reading 'status')"?
<InvestmentLayout
// ...
// This will throw an error because InvestmentProjectLocalHeader won't be called with the required
// investment prop. The <div/> will receive it instead.
localHeaderChildren={<div><InvestmentProjectLocalHeader /></div>}
>
{/* ... */}
</InvestmentLayout>I would prefer that localHeaderChildren prop was a function which would recieve the investment as an argument, which would make the dependency obvious:
<InvestmentLayout
// ...
localHeaderChildren={investment => <InvestmentProjectLocalHeader investment={investment}/>}
>
{/* ... */}
</InvestmentLayout>Or even better, if InvestmentLayout is only ever going to be used once, I would just inline the body of InvestmentProjectLocalHeader into InvestmentLayout.
There was a problem hiding this comment.
Thanks very much for this description. I was really struggling with trying to add props to the component when the component was being created before the props were available, hence the method I used. Using a function is a lot better, however I think the inline method will work for this situation as it will only be used on investment pages.
| <LocalHeaderHeading data-test="heading"> | ||
| {project.name} | ||
| </LocalHeaderHeading> | ||
| {renderChildren(project)} |
There was a problem hiding this comment.
I would just inline the body of InvestmentProjectLocalHeader here.
There was a problem hiding this comment.
Agreed, I've put in inline
| export const ProjectLayoutNew = ({ | ||
| projectId, | ||
| projectName, | ||
| pageTitle, | ||
| children, | ||
| userPermissions, | ||
| flashMessages, | ||
| }) => { | ||
| return ( | ||
| <InvestmentLayout | ||
| projectId={projectId} | ||
| heading={projectName} | ||
| pageTitle={`${pageTitle} - ${projectName} - Projects - Investments`} | ||
| breadcrumbs={buildProjectBreadcrumbs({ text: projectName })} | ||
| flashMessages={flashMessages} | ||
| useReactRouter={false} | ||
| localHeaderChildren={<InvestmentProjectLocalHeader />} | ||
| > |
There was a problem hiding this comment.
It seems that both InvestmentLayout and ProjectLayoutNew are only ever going to be used once in ProjectDetails and the only thing ProjectLayoutNew is doing is to hard code or transform some of the prop values. Wouldn't it be simpler to get rid of these components and have it all hardcoded in ProjectDetails?
There was a problem hiding this comment.
The ProjectLayoutNew will be used in all investment pages.
| margin-top: 0; | ||
| ` | ||
|
|
||
| const StyledLink = styled('a')({ |
There was a problem hiding this comment.
Shouldn't we use the govuk-react Link?
There was a problem hiding this comment.
Yeah, I've changed this. This was a legacy thing from copying over the local header component and changing it for our use
| </Breadcrumbs.Link> | ||
| ) | ||
| ) : ( | ||
| <Fragment key={breadcrumb.text}>{breadcrumb.text}</Fragment> |
There was a problem hiding this comment.
The fragment is redundant if you are only rendering a string.
There was a problem hiding this comment.
In order to prevent a unique key prop error we need to add a key. So the fragment is in to allow us to add a key to the element without actually using an html element.
| useEffect(() => { | ||
| document.title = `${pageTitle} - DBT Data Hub` | ||
| }, [pageTitle]) |
There was a problem hiding this comment.
You can now use the WatchTextContent component to also accept React vdom as pageTitle value.
There was a problem hiding this comment.
When creating this we didn't have access to the WatchTextContent. I have rebased the main, and have now implemented the WatchTextContent component
| showVerticalNav={showVerticalNav} | ||
| onShowVerticalNav={setShowVerticalNav} |
There was a problem hiding this comment.
Not a problem of this PR, but DataHubHeader should handle its state internally and not require this plumbing everywhere it is used. We can do better than that.
There was a problem hiding this comment.
Yeah, it's strange that the Datahub header itself doesn't manage this state
| breadcrumbs={ | ||
| breadcrumbs || [{ link: '/', text: 'Home' }, { text: heading }] | ||
| } |
There was a problem hiding this comment.
Is this necessary? It seems that breadcrumbs will always be defined.
There was a problem hiding this comment.
I've removed the or check
| projectId={projectId} | ||
| flashMessages={flashMessages} | ||
| breadcrumbs={ | ||
| breadcrumbs || [{ link: '/', text: 'Home' }, { text: heading }] | ||
| } | ||
| useReactRouter={useReactRouter} |
There was a problem hiding this comment.
You are just forwarding props with the same name here so you can just keep them in ...props and spread them here:
<InvestmentLocalHeader {...props}>
{localHeaderChildren}
</InvestmentLocalHeader>There was a problem hiding this comment.
refactored to use ...props
| const StyledNavWrapper = styled('div')` | ||
| margin-bottom: ${SPACING.SCALE_5}; | ||
| ` |
There was a problem hiding this comment.
Is this really necessary? I guess it's here to prevent the navigation touching the page footer, but the fundamental issue is that the footer is not sticking to the bottom of the page if the content is too small.
This should be fixed on the layout level where the layout would be wrapped in a <div/> with display: flex; flex-direction: column and its <main/> child should have flex-grow: 1 so that it pushes the footer to the bottom. The problem is that for this to work the <div/>, the <div id="react-app"/>, <body/> and <html/> need to have height: 100%.
<html style="height: 100%">
<body style="height: 100%">
<div id="react-app" style="height: 100%" >
<div style="display: flex; flex-direction: column"><!--layout wrapper-->
<header></header>
<div aria-label="local header"></div>
<main style="flex-grow: 1"></main><!--fills remaining space-->
<footer></footer>
</div>
</div>
</body>
</html>
There was a problem hiding this comment.
Had a look at this, but it's bit out of scope for this ticket. Couldn't get <div id=react-app> to take the full height even after adding height: 100%. This file is a copy of the project layout that has been tweaked and so the StyledNavWrapper is a pre existing component from that
…ce, create new layout and layout header for investments
Description of change
Change the project details page so that the layout is rendered before the resource is called. So that if there is an error then the error messages show in the layout. Create a new project layout, investment local header and investment layout component. Add props to the Investment project local header after it has been called. Inject inline resource into the new project layout component.
Test instructions
In the project details component put in a fake project id
Screenshots
Before
After
Checklist