-
Notifications
You must be signed in to change notification settings - Fork 103
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
Add Domain Page routes, layout and header #560
Conversation
export default function CadenceLogo() { | ||
return ( | ||
<svg | ||
width="32" |
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.
Setting static width would make the component non reusable.
I would suggest importing svg as link (Image) until we add the svg loader so we don't need to create svg components manually.
import { styled } from './page-header-section.styles'; | ||
import { Props } from './page-header-section.types'; | ||
|
||
export default function PageHeaderSection(props: Props) { |
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.
Do we need to create a separate section component just to handle the margin?
I don't see this reusable as not all pages have the same spacing and the spacing can change if we are going to support sticky headers.
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.
Got it, I'll define the layout directly in the domain page header instead of having this component
title: string; | ||
getContent: ( | ||
props: DomainHeaderInfoItemContentProps | ||
) => string | React.ReactElement; |
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.
React.Node
can be used as the return type.
It includes string | ReactElement |number | null
and other supported types that you usually expect as a react component return.
But since you are using it as components too, i would doubt that getContent
is a good name for the field
Also, please use React.ComponentType as it handles Class components, which is the way react works.
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.
My intention was to have the metadata item either be a string or a React element.
I can modify the type as follows:
getContent: React.ComponentType<Props> | (props: Props) => string
]} | ||
onChange={({ option }) => { | ||
if (option && option.id !== props.cluster) { | ||
const newPath = currentPath.replace( |
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 line is fragile and can be improved. We don't have any limitations for naming clusters to match any other part of the URL. Also, it's better to make it more clear which url this component is navigating to instead of manipulating the current url.
FYI: https://nextjs.org/docs/app/building-your-application/configuring/typescript#statically-typed-links
overrides={overrides.skeleton} | ||
animation={true} | ||
/> | ||
) : typeof props.content === 'string' ? ( |
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.
Please avoid nested ternaries for better readability.
We can add the rule in the linting
https://eslint.org/docs/latest/rules/no-nested-ternary
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.
Agreed, but I wanted to avoid creating another layer of hierarchy by moving the loaded state to a separate component. But I think I can handle this with styled components directly.
content: string | ReactNode; | ||
}; | ||
|
||
export type Props = (LoadingProps | LoadedProps) & { |
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.
The split here is not serving any purpose and adding complexity while using the component. [attached the usage code]
I would expect such a split in the props if the component accepts a complete set of props based on a single prop, which can help the component's user to identify which props are for each type.
But in the current case loading is just a toggle between two states. Both states will always be sent together and there is no harm in allowing that.
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.
<DomainPageHeaderInfoItem
key={configItem.title}
title={configItem.title}
{...(props.loading
? {
loading: true,
placeholderSize: configItem.placeholderSize,
}
: {
loading: false,
content: configItem.getContent({
domainInfo: props.domainInfo,
cluster: props.cluster,
}),
})}
/>
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.
Fair enough. So instead of having LoadingProps and LoadedProps, I can do the following:
type Props = {
loading: boolean;
content: React.Node;
placeholderSize: string;
}
cluster: string; | ||
}; | ||
|
||
type DomainPageHeaderInfoItem = { |
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: Maybe easier to explain what is the Item by adding a postfix for Props/Config
@@ -0,0 +1,39 @@ | |||
import React, { Suspense } from 'react'; |
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 think you forgot to rename this to exclude -layout
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 import this file in src/app/(Home)/domains/[domain]/[cluster]/layout.tsx so I left the "layout" name as is. Open to removing it though
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.
@adhityamamallan yeah i understand the reasoning behind it, but it seems that this would be the normal way of working on pages (they can be Layouts) so let's name them as they are perceived better than where they are used.
* media query utils * fix linting issues
* page tabs component * remove unused style from page-tabs * fix linting
Summary
Test plan
Unit tests, and ran cadence-web-v4 locally.
Screenshots
Loading state:
Loaded state: