-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added a list of vehicles #43
Conversation
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.
Größtenteils UI Änderungen.
Interessant sind insbesondere Website/src/app/list/page.tsx
, sowie Website/src/components/dynlist.tsx
Rein kosmetisch sind die Änderungen in Website/src/components/layout_base.tsx
, Website/src/app/layout.tsx
, Website/src/pages/_app.tsx
, und Website/src/pages/_document.tsx
Previously, each popup, even if closed was its own React root, which could have a negative performance impact.
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.
In general the structuring looks fine to allow for a modular website. Some general comments about the code:
All code should have a header comment indicating the copyright and used license, as the code from our other repositories does as well. Something like this:
/*
* KIELER RailTrail
*
* http://rtsys.informatik.uni-kiel.de/kieler
*
* Copyright 2023 by
* + Kiel University
* + Department of Computer Science
* + Real-Time and Embedded Systems Group
*
* This code is provided under the terms of the Eclipse Public License 2.0 (EPL-2.0).
*/
Please document your code. I will not do a more rigorous review, as that would require me to understand what each part does from the naming and the code alone, which I would prefer not to have to do. Please start writing code in a style where you always document the code as you write it, that is a good habit to have to write readable code that not only others, but also you will be able to understand later. We will further review the code style once there are comments.
Some more questions arising in the structure:
- why is there an app/components and a components folder? Should there be a difference between these?
- is the
pages
folder supposed to hold all pages in the end? Or do you want to organize this via their use/components? Please choose one style. lib
as a source folder name can be dangerous in the TypeScript world, as many projects compile their code into alib
folder by default and developers may be accustomed to this, maybe a better name would beutils
or similar.
children: React.ReactNode | ||
}) { | ||
return ( | ||
<div className='h-full min-h-screen flex flex-initial flex-col'> |
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.
To organize the styling between the different files all the stylings should be structured into .css files and only referenced by their class here.
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.
WON'T FIX: this sort of styling is only used for this component. There is absolutely nothing gained by defining a custom CSS class page_container
that magically applies the same style. Additionally, I would then have to look at two different files to figure out why something is styled like it is. (Also it would defy the use of tailwind for this page)
I would argue, that it also helps with maintainability, as there are no 'orphaned' CSS styles hanging around in a CSS file somewhere, that might still be in use
However, (not in this case, but maybe for some other thing) I should move some duplicated layout things into a component or a layout.tsx
file.
import Header from "@/app/components/header"; | ||
import Footer from "@/app/components/footer"; | ||
|
||
export default function 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.
A quick look through this shows me that there are practically no comments in the code and no documentation for the public API. That makes it a lot harder to understand and also review code such as this. Please comment your code.
|
||
export default RootLayout; | ||
export default function RootLayout({children,}: { children: React.ReactNode }) { |
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.
Naming here seems inconsistent: the layout_base
file contains theLayout
and the layout
file the RootLayout
.
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.
Files named layout.ts(x)
(in the app-directory) have special meaning in next.js as these define a component that "wrapps" all page components in subdirectories. As I however need to use the same layout in both the app-, as well as the pages-directory. I factored the relevant parts into a component.
However I agree that naming things is hard
init_data = (token && track_selected) ? await getInitData(token, track_id) : undefined; | ||
server_vehicles = (token && track_selected) ? await getVehicleData(token, track_id) : []; | ||
} catch (e) { | ||
console.error('Catched e:', e); |
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.
Error messages should be a bit more informative about the context in which they occurred.
Some thoughts on the general comments, also why some of these are impossible to resolve without switching the currently used framework (nextjs) Next does routing based on directory structure. The modern way to do this is the However, I should probably move all components into the I will also try to add more comments, however we could probably start a long discussion about the amount of comments that are necessary. I guess we both want to avoid that. |
fixes #38, closes #38