-
Notifications
You must be signed in to change notification settings - Fork 7
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
change project details to render the layout before loading the resour… #6454
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
import React, { Children, Fragment, cloneElement } from 'react' | ||
import PropTypes from 'prop-types' | ||
import styled from 'styled-components' | ||
import { SPACING } from '@govuk-react/constants' | ||
import Main from '@govuk-react/main' | ||
import Breadcrumbs from '@govuk-react/breadcrumbs' | ||
import { Link } from 'react-router-dom' | ||
|
||
import { GREY_4 } from '../../../client/utils/colours' | ||
import LocalHeaderHeading from '../LocalHeader/LocalHeaderHeading' | ||
import FlashMessages from '../LocalHeader/FlashMessages' | ||
import { InvestmentResource } from '../Resource' | ||
import urls from '../../../lib/urls' | ||
|
||
// Using <div> as there is already a <header> on the page | ||
// role="region" gives the element significance as a landmark | ||
const StyledHeader = styled('div')` | ||
padding-bottom: ${SPACING.SCALE_5}; | ||
background-color: ${GREY_4}; | ||
padding-top: ${SPACING.SCALE_3}; | ||
` | ||
|
||
const StyledMain = styled(Main)` | ||
padding-top: 0; | ||
` | ||
|
||
const BreadcrumbsWrapper = styled(Breadcrumbs)` | ||
margin-bottom: ${SPACING.SCALE_5}; | ||
margin-top: 0; | ||
` | ||
|
||
const StyledLink = styled('a')({ | ||
fontSize: 20, | ||
display: 'inline-block', | ||
fontFamily: 'Arial, sans-serif', | ||
marginTop: 8, | ||
marginBottom: 8, | ||
}) | ||
|
||
/** | ||
* The generic local header component. | ||
*/ | ||
const InvestmentLocalHeader = ({ | ||
breadcrumbs, | ||
flashMessages, | ||
projectId, | ||
children, | ||
useReactRouter = false, | ||
}) => { | ||
const renderChildren = (project) => { | ||
return Children.map(children, (child) => { | ||
return cloneElement(child, { | ||
investment: project, | ||
}) | ||
}) | ||
} | ||
Comment on lines
+50
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would avoid this mechanism as it makes components that are dependent on each other appear to be independent. In this particular case, <InvestmentLayout
// ...
localHeaderChildren={<InvestmentProjectLocalHeader />}
>
{/* ... */}
</InvestmentLayout> One day anothe developer might want to tweak the markup (for whatever reasons) and wrap the <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 <InvestmentLayout
// ...
localHeaderChildren={investment => <InvestmentProjectLocalHeader investment={investment}/>}
>
{/* ... */}
</InvestmentLayout> Or even better, if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
return ( | ||
<StyledHeader | ||
aria-label="local header" | ||
data-auto-id="localHeader" | ||
data-test="localHeader" | ||
role="region" | ||
> | ||
<StyledMain> | ||
<BreadcrumbsWrapper data-test="breadcrumbs"> | ||
{breadcrumbs?.map((breadcrumb) => | ||
breadcrumb.link ? ( | ||
useReactRouter && breadcrumb.text !== 'Home' ? ( | ||
<Breadcrumbs.Link | ||
as={Link} | ||
key={breadcrumb.link} | ||
to={breadcrumb.link} | ||
> | ||
{breadcrumb.text} | ||
</Breadcrumbs.Link> | ||
) : ( | ||
<Breadcrumbs.Link key={breadcrumb.link} href={breadcrumb.link}> | ||
{breadcrumb.text} | ||
</Breadcrumbs.Link> | ||
) | ||
) : ( | ||
<Fragment key={breadcrumb.text}>{breadcrumb.text}</Fragment> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fragment is redundant if you are only rendering a string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
) | ||
)} | ||
</BreadcrumbsWrapper> | ||
<FlashMessages flashMessages={flashMessages} /> | ||
<InvestmentResource id={projectId}> | ||
{(project) => ( | ||
<> | ||
<StyledLink | ||
data-test="heading-link" | ||
href={urls.companies.detail(project.investorCompany.id)} | ||
> | ||
{project.investorCompany.name} | ||
</StyledLink> | ||
<LocalHeaderHeading data-test="heading"> | ||
{project.name} | ||
</LocalHeaderHeading> | ||
{renderChildren(project)} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just inline the body of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I've put in inline |
||
</> | ||
)} | ||
</InvestmentResource> | ||
</StyledMain> | ||
</StyledHeader> | ||
) | ||
} | ||
|
||
InvestmentLocalHeader.propTypes = { | ||
/** | ||
* Contains the breadcrumbs | ||
*/ | ||
breadcrumbs: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
link: PropTypes.string, | ||
text: PropTypes.oneOfType([PropTypes.string, PropTypes.element]) | ||
.isRequired, | ||
}) | ||
), | ||
/** | ||
* Contains the flash messages | ||
*/ | ||
flashMessages: PropTypes.shape({ | ||
type: PropTypes.oneOfType([ | ||
PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
body: PropTypes.string.isRequired, | ||
heading: PropTypes.string.isRequired, | ||
id: PropTypes.string, | ||
}) | ||
), | ||
PropTypes.arrayOf(PropTypes.string).isRequired, | ||
]), | ||
}), | ||
/** | ||
* Contains the heading text to be displayed | ||
*/ | ||
heading: PropTypes.oneOfType([PropTypes.string, PropTypes.node]), | ||
|
||
/** | ||
* Contains the subheading text to be displayed | ||
*/ | ||
subheading: PropTypes.oneOfType([PropTypes.string, PropTypes.node]), | ||
/** | ||
* Contains a link that appears above the heading | ||
*/ | ||
headingLink: PropTypes.shape({ | ||
url: PropTypes.string.isRequired, | ||
text: PropTypes.string.isRequired, | ||
}), | ||
/** | ||
* Contains an item that renders above the heading (in the same position as the headingLink) | ||
*/ | ||
superheading: PropTypes.node, | ||
/** | ||
* Contains an item that renders below the heading | ||
*/ | ||
children: PropTypes.node, | ||
} | ||
|
||
export default InvestmentLocalHeader |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,11 +79,11 @@ const InvestmentProjectLocalHeader = ({ investment }) => ( | |
<MetaListItem text="Created on"> | ||
{formatMediumDateTime(investment.createdOn)} | ||
</MetaListItem> | ||
{investment.createdBy?.ditTeam?.name && ( | ||
{investment.createdBy?.ditTeam?.name ? ( | ||
<MetaListItem text="Created by"> | ||
{investment.createdBy.ditTeam.name} | ||
</MetaListItem> | ||
)} | ||
) : null} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this really necessary? The optional chaining operator will evaluates to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed that back as you are correct that it isn't required for this scenario. |
||
</MetaList> | ||
<ThemeProvider theme={timelineTheme}> | ||
<Timeline | ||
|
@@ -108,7 +108,7 @@ InvestmentProjectLocalHeader.propTypes = { | |
/** | ||
* An investment project | ||
*/ | ||
investment: PropTypes.object.isRequired, | ||
investment: PropTypes.object, | ||
} | ||
|
||
export default InvestmentProjectLocalHeader |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import React, { useEffect, useState } from 'react' | ||
import { createGlobalStyle } from 'styled-components' | ||
import PropTypes from 'prop-types' | ||
import GridCol from '@govuk-react/grid-col' | ||
import GridRow from '@govuk-react/grid-row' | ||
|
||
import Footer from '../Footer' | ||
import Main from '../Main' | ||
import DataHubHeader from '../DataHubHeader' | ||
import { InvestmentLocalHeader } from '../../components' | ||
|
||
const GlobalStyles = createGlobalStyle` | ||
*, *:before, *:after { | ||
box-sizing: initial; | ||
} | ||
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary? It would be good to add an explanation comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is a copy of the default layout that has been tweaked. We don't use the default layout at all for these pages, so this is just adding the global styling that we get if use the default layout |
||
` | ||
|
||
const InvestmentLayout = ({ | ||
projectId, | ||
heading, | ||
pageTitle, | ||
flashMessages, | ||
breadcrumbs, | ||
children, | ||
useReactRouter = false, | ||
localHeaderChildren, | ||
}) => { | ||
const [showVerticalNav, setShowVerticalNav] = useState(false) | ||
useEffect(() => { | ||
document.title = `${pageTitle} - DBT Data Hub` | ||
}, [pageTitle]) | ||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can now use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When creating this we didn't have access to the WatchTextContent. I have rebased the main, and have now implemented the WatchTextContent component |
||
return ( | ||
<> | ||
<GlobalStyles /> | ||
<DataHubHeader | ||
showVerticalNav={showVerticalNav} | ||
onShowVerticalNav={setShowVerticalNav} | ||
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a problem of this PR, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's strange that the Datahub header itself doesn't manage this state |
||
/> | ||
<InvestmentLocalHeader | ||
projectId={projectId} | ||
flashMessages={flashMessages} | ||
breadcrumbs={ | ||
breadcrumbs || [{ link: '/', text: 'Home' }, { text: heading }] | ||
} | ||
Comment on lines
+42
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this necessary? It seems that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed the or check |
||
useReactRouter={useReactRouter} | ||
Comment on lines
+40
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are just forwarding props with the same name here so you can just keep them in <InvestmentLocalHeader {...props}>
{localHeaderChildren}
</InvestmentLocalHeader> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. refactored to use ...props |
||
> | ||
{localHeaderChildren} | ||
</InvestmentLocalHeader> | ||
<Main> | ||
<GridRow> | ||
<GridCol>{children}</GridCol> | ||
</GridRow> | ||
</Main> | ||
<Footer /> | ||
</> | ||
) | ||
} | ||
|
||
InvestmentLayout.propTypes = { | ||
heading: PropTypes.oneOfType([PropTypes.string, PropTypes.node]).isRequired, | ||
headingLink: PropTypes.shape({ | ||
url: PropTypes.string.isRequired, | ||
text: PropTypes.string.isRequired, | ||
}), | ||
subheading: PropTypes.oneOfType([PropTypes.string, PropTypes.node]), | ||
pageTitle: PropTypes.string.isRequired, | ||
children: PropTypes.oneOfType([PropTypes.string, PropTypes.node]), | ||
} | ||
|
||
export default InvestmentLayout |
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.
Shouldn't we use the govuk-react
Link
?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.
Yeah, I've changed this. This was a legacy thing from copying over the local header component and changing it for our use