2023-04-25: Feedback zur Abgabe #36
Conversation
feat: Add Component library
fix: Use tailwind styles in new app
feat: #2 Home-Screen: Alle Posts laden und speichern (API-Call)
# Conflicts: # package-lock.json
…liting feat: Added eslint, prettier, dependency-cruiser, smartive/eslint and smart…
# Conflicts: # pages/index.tsx # pages/login.tsx
feat: #3 Home-Screen: Navbar
# Conflicts: # pages/login.tsx
# Conflicts: # src/pages/login.tsx
feat: Added src folder as parent
Feature/app 25 Add Middleware
…-view # Conflicts: # src/pages/profile/[id].tsx
…pload # Conflicts: # src/services/posts.ts
…load feat: Added comments file upload
Feature/app 31 mobile view
#23 Comment als Link oder als Label hinzufügen.
feature/app-37 Add Readme
fix: Added new storybook
Feature/app 46 readme
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| "icons": [ | ||
| { | ||
| "src": "/app-icon-160-160.png", | ||
| "sizes": "192x192", |
There was a problem hiding this comment.
Die angegebene Grösse sollte mit der eigentlichen Icon Grösse übereinstimmen um Verzerungen zu verhindern.
| import { Oval } from 'react-loader-spinner'; | ||
|
|
||
| interface CurrentUser { | ||
| user?: User; |
There was a problem hiding this comment.
User ist im Kontext verfügbar (useSession) und braucht nicht über Props in eine Komponente gegeben zu werden.
| {session && ( | ||
| <Navbar logoHref={'/'} logoAriaLabel={'Navigate to home'}> | ||
| <span className={'absolute top-0'}> | ||
| <a href={href} onClick={(e) => handleClick(e)}> |
There was a problem hiding this comment.
Macht nicht wirklich Sinn das Routing über href und über onClick mit router.push auszuführen. Die onClick Variante isch schlecht für Accessibility. Ebenfalls sollte next-link verwendet werden.
| isFileSelected={state.isFileSelected} | ||
| currentFile={state.currentFile} | ||
| errorMessage={state.errorMessage} | ||
| onAddFile={(f) => onHandleFile(f)} |
There was a problem hiding this comment.
Wenn ihr Funktionen ausserhalb von JSX definiert müsst ihr hier keine ad-hoc Funktionen mehr erstellen, das braucht nur zusätzliches Memory und nützt nichts.
onAddFile={(f) => onHandleFile(f)} --> onAddFile={onHandleFile}
onClose={() => onAbort()} --> onClose={onAbort}
| interface MumbleCard { | ||
| mumble: Mumble; | ||
| showComments?: boolean; | ||
| commentSubmitted?: (newReply: Reply) => void; |
There was a problem hiding this comment.
Sollte aus Konsistenzgründen onCommentSubmitted sein.
| <MumbleList | ||
| mumbles={activeTab === 'mumbles' ? mumbles : likedMumbles} | ||
| users={users} | ||
| totalMumbles={activeTab === 'mumbles' ? count : likedCount} | ||
| key={activeTab === 'mumbles' ? 'mumbles' : 'likes'} | ||
| mumbleKey={activeTab === 'mumbles' ? 'mumbles' : 'likes'} | ||
| userId={profileUser.id} | ||
| ></MumbleList> |
There was a problem hiding this comment.
Ihr solltet der Mumble List als key die user-id geben. Damit stellt ihr sicher, dass die Komponente neu erstellt wird wenn via Client-side Transition das Profil gewechselt wird (z.B. wenn ich auf einem fremden Profil bin und dann via Header auf mein eigenes navigiere).
Weil ihr das aktuell nicht macht und die MumbleList Komponente die Mumbles in einem internen State speichert, sehe ich beim Wechsel auf mein eigenes Profil immer noch die Mumbles des fremden Profils bis ich den Tab wechsle.
| ...state, | ||
| copiedActive: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Es ist hilfreich einen default case zu verwenden um einen Error auszugeben, wenn ein action.type verwendet wird, welcher nicht existiert. Das hilft bei der Entwicklung oder Refactorings zu verhindern, das aus versehen Typos zu Bugs führen.
| accessToken: session?.accessToken, | ||
| }); | ||
| } else { | ||
| reloadedMumbles = await fetchMumbles({ limit: 10, offset: state.nextOffset, creator: userId as string }); |
There was a problem hiding this comment.
Zumindest auf der Startseite im Feed ist es problematisch mit offset die load-more Funktionalität einzubauen. Andere User könnten bereits weitere Mumbles erstellt haben, wodurch der offset in euerem State nicht mehr stimmt.
Wurden also im Hintergrund von anderen Usern 3 Mumbles erstellt, würdet ihr mit eurem Offset 3 Mumbles welche bereits geladen sind nochmals laden. Das ist für den User verwirrend und kann sogar zu Fehlern führen, da ihr dann in der Liste mehre Komponenten mit der gleichen ID als Key rendert.
| export function convertToUploadAction(upload: UploadState) { | ||
| switch (upload.type) { | ||
| case 'upload_drag_active': { | ||
| return 'upload_drag_active'; | ||
| } | ||
| case 'upload_drag_leave': { | ||
| return 'upload_drag_leave'; | ||
| } | ||
| case 'upload_error_files': { | ||
| return 'upload_error_files'; | ||
| } | ||
| case 'upload_error_max_size': { | ||
| return 'upload_error_max_size'; | ||
| } | ||
| case 'upload_error_wrong_type': { | ||
| return 'upload_error_wrong_type'; | ||
| } | ||
| case 'upload_file_reset': { | ||
| return 'upload_file_reset'; | ||
| } | ||
| case 'upload_file_success': { | ||
| return 'upload_file_success'; | ||
| } | ||
| default: | ||
| return 'upload_file_reset'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Der Sinn dieser Funktion erschliesst sich mir nicht ganz da es eigentlich ein Eins-zu-Eins mapping ist mit default Value.
|
|
||
| try { | ||
| const { count, mumbles } = await fetchMumbles({ limit: 10 }); | ||
| const { users } = await fetchUsers({ accessToken: session?.accessToken }); |
There was a problem hiding this comment.
Aktuell funktioniert der Ansatz, immer alle User zu laden und dem Client mitzugeben ganz ok. Falls sich aber ein paar tausend User registrieren würden, würde die Performance euerer Seite stark leiden, da der Client immer mehrere Megabyte an JSON runterladen müsste für Userdaten welche wahrscheinlich gar nicht gebraucht werden.
No description provided.