this is the assignment of Michail Apostolou #6

cucaracha18421921 · 2024-09-24T13:14:56Z

No description provided.

wiz-gwi · 2024-09-24T13:15:10Z

Wiz Scan Summary

Scan Module						Total
IaC Misconfigurations	0	0	0	0	0	0
Vulnerabilities	0	1	1	0	0	2
Sensitive Data	0	0	0	0	0	0
Secrets	0	0	0	0	0	0

Total	0	1	1	0	0	2

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

chbardamu · 2024-09-28T09:11:50Z

cat-loving-app/src/cats/CatDetail.tsx

+                    <>
+                        <DescriptionStyled>{description}</DescriptionStyled>
+                        { id!== '' && 
+                            <button onClick={(evt) => {


Why do you have to use event.stopPropagation? The CatDetailContainer has an action-less onClick event attached.

This was interfering with the modal. CatDetail can be rendered either inside a modal or just as a single page. In the case of a modal the event propagating up to the modal level and closing it as a result

Hmm... Maybe that's because you have that empty event listener that is attached to the parent component. Otherwise, I can't see how the modal can be closed by clicking on part of its content.

chbardamu · 2024-09-28T09:12:17Z

cat-loving-app/src/cats/CatDetail.tsx

+        fetchCatDescription();
+    }, [catId]);
+
+    const handleAddToFavorites = useCallback(async () => {


How would you write that if you wanted to include the event as an argument?

I would change line 110 to
const handleAddToFavorites = useCallback(async (evt: React.MouseEvent) => {

and line 161 to
handleAddToFavorites(evt);

chbardamu · 2024-09-28T09:12:20Z

cat-loving-app/src/cats/CatDetail.tsx

+    const [showSucessMessage, setShowSucessMessage] = useState<boolean>(false);
+
+    useEffect(() => {
+        const fetchCatDescription = async () => {


How come you chose to not use useCallback for this one, like you did with handleAddToFavorites?

I need to define this function inside useEffect, so I can use it when the component renders.

I can't put it in a useCallback since if you want your function to run from within a hook you have to define it inisde the hook. Inside the hook I can't use useCallback hook. I can't define the function outside as the only way to run an async function from the useEffect hook is to define within the hook.

As a result I can't memoize this function. If there is another way to achieve that I would be really happy to update it

It might be I'm missing something, but I had in mind something like you already do here:

https://github.com/GlobalWebIndex/platform-react-challenge/pull/6/files/2121a0cd5d72a8b83ea655f3f22713a0b1567916#diff-8fb4d02791c9e946c016e1ae31c2bdb4aaddf865b51596f1cd557b64fc3aed23R25-R36

chbardamu · 2024-09-28T09:13:37Z

cat-loving-app/src/cats/CatDetail.tsx

+    let {catId} = useParams();
+    if(catId === undefined && id !== '' ) {
+        catId = id;
+    }


const catId = useParams().catId || props.catId;

...would be much clearer and indicate the origin of the catId value, which now is a bit obfuscated.

Indeed it is much cleaner. This was missed. For debugging I had it expanded and didn't simplify it

chbardamu · 2024-09-28T09:14:21Z

cat-loving-app/src/cats/CatDetail.tsx

+    margin-top: 10px;
+`;
+
+const CatDetailContainer = styled.div<{ $withModal?: boolean }>`


Plain curiosity: Why using a '$' in naming? Angular reference? 😋

Reason for that is in order to know at first glance that this prop is used only for styling

Hm. I don't get this. You use a styled component - what else could it be used for if not for styling?

When we render CatDetailContainer the variable withModal is only used for styling and doesn't play any part on the logic, that's why I named it starting with an $. But of course this depends on the code styling agreements

chbardamu · 2024-09-28T09:18:09Z

cat-loving-app/src/common/CatImage.tsx

+        <li data-testid="cat-image-container" key={catId} style={{ width: `${100 / imagesPerRow}%`, display: 'flex', justifyContent: 'center', alignItems: 'center' }}>
+            <div


No styled component?

this was fixed on last commit

chbardamu · 2024-09-28T09:18:59Z

cat-loving-app/src/cats/RandomCatsScreen.tsx

+const CatImageListContainer = styled.div`
+    position: relative;
+`;
+
+const CatImageContainer = styled.ul`
+    display: flex; 
+    flex-wrap: wrap;
+`;


I noticed you do not separate styled components from regular ones, file-wise. Is there a reason for that?

As you can see before I had create a seperate component for styles of the index. I didn't do it in this case since my thoughts were that since these components are not re-used and they are not that many cluttering the file it would be easier to navigate on the project at its current size.

Of course for readability reasons and to make the testing coverage more precise, I would place them into {componentName}.styles.tsx files as the project was growing

It's not just this case however, it's every other case. I understand reusability being a factor, but I'd still argue that the practice of keeping your styled components inside the same file is already causing a maintenance issue. Although, it is indeed sensible to make such changes as the app is growing, setting up a good foundation is essential, and I think this file separation is part of this foundation. Best practices are mostly for things that we may need to deal with later on, aren't they?

I totally agree with this comment and I will be happy to implement it. It's an approach I agree, but given the time this was a detail I left for later, but as you mentioned it is already reaching the point where readability is already hurt

chbardamu · 2024-09-28T15:40:08Z

cat-loving-app/src/cats/RandomCatsScreen.tsx

+            <RandomCatsTop 
+                header="Random Cats"
+                imagesPerRow={imagesPerRow} 
+                setImagesPerRow={setImagesPerRow}  
+                getNewListOfCats={async ()=> {
+                    localStorage.removeItem('cats');
+                    const newCats = await getRandomCats();
+                    setCats(newCats);
+                    localStorage.setItem('cats', JSON.stringify(newCats));
+                }}
+            />


Could you explain the logic of this component and why you choose to do it this way?

I noticed this component does not follow common conventions, like the "getNewListOfCats", which appears to be an event ('onclick') action callback to a child component. It'd be expected to name it accordingly (eg. "onDisplayMoreCats").

This component is the one that is the container of the screen that shows random Cats. The logic here is that the RandomCatsTop contains the ui that sets the number of images per row and gets a fresh list of cats.

This is necessary since I store in localStorage every request for 10 random cats. I store the requests in localStorage in order to make more seamless the experience of a user opening the CatDetail inside the App and not in a modal. In that case when the user was hitting back he/she would get a new set of 10 cats. For that reason I have implemented the button getNewListOfCats in order to clean your local storage

I am totally in agreement that it should be named with a name starting with 'on' to indicate the action

I see. In that case, you think using localStorage is better than sessionStorage?

How would you clear the history of loaded cat images for the user to start all anew when they reopen the app? Assuming this was an actual spec/requirement.

I think you are right. SessionStorage should be better as long there's no strict requirement needing to persist data even after the tab is closed.

I could use localStorage.removeItem(key); This looks implicit and it only removes the specific item.

chbardamu · 2024-09-30T16:13:46Z

cat-loving-app/src/cats/CatDetail.tsx

+            setShowSucessMessage(true);
+            setShowErrorMessage(false);
+        }else{
+            setShowErrorMessage(true);
+            setShowSucessMessage(false);


Isn't it a bit redundant to have to vars on the component's state for something that can easily be done using just one? Let alone the unnecessary re-renderings this causes.

I would change lines 164, 165 to
{ message !== null && message == 'failed' && <InformationText>Could not save to favorites</InformationText>} {message !== null && message == 'success' && <InformationText>Saved to favorites</InformationText>}

I would remove lines 89, 90 in favor of const [message,setMessage] = useState<string>('');

and lines 112-118 will be replaced by

setMessage((successFullySaved?'success':'failed'));

Naming is a bit generic, but for the shake of passing the idea let's call the variable message

chbardamu · 2024-09-30T16:28:56Z

cat-loving-app/src/services/breedsService.ts

+    }
+};
+
+const getDataForBreed = async ( breedId?: string ):Promise<BreedDetails | undefined> => {


Can this function ever return undefined?

yes if it throws an exception

chbardamu · 2024-09-30T16:29:44Z

cat-loving-app/src/services/breedsService.ts

+            headers: {
+                'x-api-key': 'live_Ka5tWZF7c0Gm8UhINCls6XSG8wmnHHVyxF8gFnQcnXXl4zPTA8RZcAWSsnpCphEm'


Why did you choose to use this api key this way? And by that, I mean directly inside each of the functions that require it to perform the corresponding requests.

Yes. It was not in the requirement any detail on the authentication of that or how to retrieve the key. If we had a single key that was represented by our service I would put it as an env variable that I could set on my deployment pipeline. This variable can also be a secret ( like it is possible with GithubActions )

chbardamu · 2024-09-30T16:47:16Z

cat-loving-app/src/breeds/BreedDetail.tsx

+                            alt = {`Breed ${breed.name} ${index + 1}`} style={{ maxWidth: '100%', height: 'auto' }} 
+                            onClick = {() => {


You fallback to inline styling instead of using styled components in several cases. Why is that?

chbardamu · 2024-09-30T16:49:29Z

cat-loving-app/src/cats/RandomCatsScreen.tsx

+                const newCats = await getRandomCats();
+                const allCats = [...cats, ...newCats];
+                setCats(allCats);
+                localStorage.setItem('cats', JSON.stringify(allCats));


This is a bit dirty. You could use a separate callback.

Indeed I should have extracted into a function that is wrapped with useCallback. This was missed as well

chbardamu · 2024-09-30T16:55:49Z

I see you haven't used a code formatter. Is there a reason for that?

this is the assignment of Michail Apostolou

5814e60

vulnerabilities fix

2121a0c

chbardamu reviewed Sep 28, 2024

View reviewed changes

review feedback updates

31bc37f

chbardamu reviewed Sep 30, 2024

View reviewed changes

		<li data-testid="cat-image-container" key={catId} style={{ width: `${100 / imagesPerRow}%`, display: 'flex', justifyContent: 'center', alignItems: 'center' }}>
		<div

		headers: {
		'x-api-key': 'live_Ka5tWZF7c0Gm8UhINCls6XSG8wmnHHVyxF8gFnQcnXXl4zPTA8RZcAWSsnpCphEm'

		alt = {`Breed ${breed.name} ${index + 1}`} style={{ maxWidth: '100%', height: 'auto' }}
		onClick = {() => {

this is the assignment of Michail Apostolou #6

Are you sure you want to change the base?

this is the assignment of Michail Apostolou #6

Conversation

cucaracha18421921 commented Sep 24, 2024

wiz-gwi bot commented Sep 24, 2024

Wiz Scan Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbardamu Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cucaracha18421921 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbardamu Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbardamu Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbardamu Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chbardamu commented Sep 30, 2024

chbardamu Sep 30, 2024 •

edited

Loading

cucaracha18421921 Sep 30, 2024 •

edited

Loading

chbardamu Sep 28, 2024 •

edited

Loading

chbardamu Sep 30, 2024 •

edited

Loading

chbardamu Sep 30, 2024 •

edited

Loading