-
Notifications
You must be signed in to change notification settings - Fork 14
Add HeaderComponent #27
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
base: main
Are you sure you want to change the base?
Conversation
|
||
import Status from "../statusComponent/StatusComponent"; | ||
|
||
import { StatusProps } from "../statusComponent/type"; |
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.
import Status from "../statusComponent/StatusComponent"; | |
import { StatusProps } from "../statusComponent/type"; | |
import Status from "../statusComponent/StatusComponent"; | |
import { StatusProps } from "../statusComponent/type"; |
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.
ok
front/src/app/page.tsx
Outdated
@@ -2,12 +2,11 @@ | |||
|
|||
import { useState } from "react"; | |||
import useWebSocket from "react-use-websocket"; | |||
|
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.
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.
ok
<div className="flex flex-row justify-center"> | ||
<div className="pt-10 pb-20 px-10"> | ||
<div className="flex flex-row min-w-fit justify-center flex-nowrap text-nowrap"> | ||
<div className="basis-1/4 flex flex-col justify-center items-center"> |
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 there's some magic in there which broke the page center alignment when being removed.
d66ca15
to
9d14042
Compare
server: Server; | ||
} | ||
|
||
// Define the color and icon the status column |
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.
// Define the color and icon the status column | |
// Define color and icon of the status column |
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.
ok
|
||
useEffect(() => { | ||
if (!dataEquality) { | ||
console.log("change detected"); |
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.
console.log("change detected"); |
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.
ok
|
||
return ( | ||
servers.map((server) => ( | ||
<tr ref={React.createRef()} key={category + server.planCode} className={`${changedRows.has(server.planCode) ? 'bg-yellow-200' : 'transition duration-1000 delay-150 even:bg-blue-300 odd:bg-blue-100'} font-mono`}> |
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 keep the odd/even colors.
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.
ok
lastMessage, | ||
}: StatusProps) { | ||
return ( | ||
<div className="HeaderComponent flex justify-evenly w-full"> |
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.
What is this HeaderComponent
class used for ?
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.
removed ok
const ordered: CategoryOrder = { ...categoryOrder, ...serversByCategory }; | ||
|
||
return ( | ||
<table className="text-nowrap border-separate border-spacing-x-0 border-spacing-y-4"> |
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.
No need for vertical spacing, this table is already quite big.
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.
minimum to spacing-y-2
<td>{server.storage}</td> | ||
<td>{server.bandwidth}</td> | ||
<td>{`${server.price} ${server.currencyCode}`}</td> | ||
<td className="flex justify-end py-6"> |
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.
Why is this py-6
here instead of the tr
? This should also be reduced to reduce vertical space used.
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.
not working on tr but removed
const serversByCategory = Object.groupBy(data, (server: Server) => server.category); | ||
|
||
// Merge the server and respect the categories order | ||
const ordered: CategoryOrder = { ...categoryOrder, ...serversByCategory }; |
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.
It would be nice to filter out categories with no servers
const ordered: CategoryOrder = { ...categoryOrder, ...serversByCategory }; | ||
|
||
return ( | ||
<table className="text-nowrap border-separate border-spacing-x-0 border-spacing-y-2"> |
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 would rather not have the border-spacing-y-0
it makes reading the table harder I find
<table className="text-nowrap border-separate border-spacing-x-0 border-spacing-y-2"> | |
<table className="text-nowrap border-separate border-spacing-x-0"> |
lastMessage, | ||
}: StatusProps) { | ||
return ( | ||
<div className="flex justify-evenly w-full"> |
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 would suggest different spacing for the header with ...
<div className="flex justify-evenly w-full"> | |
<div className="flex justify-evenly w-full p-2"> |
<thead> | ||
<tr> | ||
{columnsHead.map((columnHead) => ( | ||
<th key={columnHead} className="p-4"> |
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.
... and
<th key={columnHead} className="p-4"> | |
<th key={columnHead} className="p-2"> |
6fbd3e8
to
e96f759
Compare
Move the page header into a new HeaderComponent.