Ddfbra 294 hent paragraph data fra cms og vis i go fe #94
Conversation
app/[...slug]/page.tsx
Outdated
|
|
||
| if (routeType === "RouteRedirect") { | ||
| // TODO: implement redirect | ||
| return |
There was a problem hiding this comment.
What happens when you return nothing?
There was a problem hiding this comment.
It will return 'undefined' which is a falsy statement and therefore not render anything. But lets return null instead to keep consistency.
| return ( | ||
| <div className="flex flex-col gap-y-paragraph-spacing"> | ||
| <ParagraphResolver paragraphs={paragraphs ?? []} /> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
I think that if we beforehand know that there is no data, in this case paragraphs I think we should not to to resolve pragraphs.
So it would be:
if (!paragraphs) {
// Null or something else default.
return nulll;
}
return (
<div className="flex flex-col gap-y-paragraph-spacing">
<ParagraphResolver paragraphs={paragraphs} />
</div>
)There was a problem hiding this comment.
What kind of rendering do we want if there are no paragraphs?
There was a problem hiding this comment.
In my opinion, If theres no paragraphs we dont want to show anything.
| const [mounted, setMounted] = React.useState(false) | ||
|
|
||
| useEffect(() => { | ||
| setMounted(true) |
There was a problem hiding this comment.
Why do we need this? Please document why you need to keep in a state if the compoment is mounted.
There was a problem hiding this comment.
Im pretty sure I implemented it because it had some buggy behavior on the demo site. But lets try to remove it and se if the bug is still present.
app/[...slug]/page.tsx
Outdated
| if (routeType === "RouteExternal") { | ||
| // TODO: implement external route redirect | ||
| return | ||
| } |
There was a problem hiding this comment.
In general I am not fan of code stubs if they are not needed. Could we maybe just write one todo where we write that we need to take care of these two cases?
Consider creating a Jira task for it too.
There was a problem hiding this comment.
Typescript is complaining if im not checking for each routeType coming from the cms
codegen.ts
Outdated
| @@ -6,6 +6,7 @@ const { loadEnvConfig } = require("@next/env") | |||
|
|
|||
| loadEnvConfig(process.cwd()) | |||
|
|
|||
| process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0" | |||
There was a problem hiding this comment.
Please write a comment why this is needed.
There was a problem hiding this comment.
Actually it should not be needed when you have inserted this:
https://github.com/danskernesdigitalebibliotek/dpl-go/pull/94/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R12
So it should be either or.
There was a problem hiding this comment.
Im removing this line 😊
| {/* @ts-ignore TODO: figure out how to type dynamically imported components */} | ||
| <DynamicComponentType key={index} {...paragraph} /> |
There was a problem hiding this comment.
While I think it is ok, for now, to ignore the ts error, I really think we should be able to handle case like this. And it is definetely possible.
If you choose to ignore this I think we should have a jira task for it, otherwise it will will be forgotten in eternity ....eternity ... eternity (weird echo)
| if (DynamicComponentType === null) return null | ||
|
|
||
| return ( | ||
| <ParagraphErrorBoundary key={index}> |
There was a problem hiding this comment.
Nice that you introduce a Paragraph Error boundary.
Is we talked about earlier we should have a clear defined way of treating errors throughout the application.
Please write an ADR about what the idea is behind treating errors in paragraphs. Or if you have a general idea of handling errrors in components with error boundaries the ADR can contain information about that too.
| <Button className="mt-4" onClick={resetErrorBoundary}> | ||
| Genindlæs | ||
| </Button> |
There was a problem hiding this comment.
Do we want a button for reloading failed content?
There was a problem hiding this comment.
I think if there's a onClick method on the element it should be a button. Maybe we could remove the button entirely?
The resetErrorBoundary is in case you want to try reloading the element. Maybe a connection timeout or something happend and the paragraph did not load in time.
package.json
Outdated
| "format:check": "prettier --check .", | ||
| "format:write": "prettier --write .", | ||
| "lint": "next lint", | ||
| "start:with-server-source-maps": "NODE_OPTIONS='--enable-source-maps=true' next start", | ||
| "start": "next start", | ||
| "start": "NODE_EXTRA_CA_CERTS=\"$(mkcert -CAROOT)/rootCA.pem\" next start", |
There was a problem hiding this comment.
This should not be inserted here, since we use start in. production
Link to issue
https://reload.atlassian.net/browse/DDFBRA-278
Description
This PR introduces: