frickerg / Web-Engineering-3

MAS SE (CAS 3) 2024
0 stars 0 forks source link

Feature/64 feedback bemerkung 4 #86

Closed jvanf closed 5 days ago

jvanf commented 1 week ago

Refactored styled components so that they are not wrapped in a component again

jvanf commented 1 week ago

@frickerg @g0tcoffee

Wer gerade Zeit hat, bitte review von "Next up" schnappen und sich als reviewer selber zuweisen. Unbedingt auschecken und testen. Bei mir funktionierte alles und auch im browser gab es keine runtime fehler.

Danke :)

jvanf commented 1 week ago

Bei der Label-Komponente bin ich mir nicht sicher, ob diese nicht auch angepasst werden sollte.

Man könnte StyledLabel direkt exportieren als Label aber man müsste dann im Sortheader folgende Anpassung vornehmen:

 export default function SortHeader(props: Readonly<SortHeaderProps>) {
+  const getSortIndicator = (type: InputType) => {
+    if (props.sortType !== type) return ''
+    return props.sortDirection === 'asc' ? '▲' : '▼'
+  }
+
   return (
     <SimpleTableHeader>
       <TableHeaderContainer>
         {sortableTypes.map(type => (
-          <Label
-            key={type}
-            label={capitalize(type)}
-            onClick={() => props.handleSortSelection(type)}
-            isSorted={props.sortType === type}
-            sortDirection={props.sortDirection}
-          />
+          <Label key={type} onClick={() => props.handleSortSelection(type)}>
+            {capitalize(type)} {getSortIndicator(type)}
+          </Label>
         ))}
       </TableHeaderContainer>
     </SimpleTableHeader>

Dabei würde die Funktion getSortIndicator, welche in der Label-Komponente plaziert ist, in die SortHeader-Komponente versetzt werden.

Was meinst du/ihr?

frickerg commented 5 days ago

Yo @jvanf zu deiner Frage in den Comments: finde ich auch eine gute idee, da so die optionalen properties aus dem Label abgebaut werden können und bei der Komponente liegen, bei der sie wirklich gebraucht werden. Labels werden aktuell nur vom SortHeader verwendet, darum kann der change noch recht einfach integriert werden :)