Skip to content
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

ArtCanvas #54

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

ArtCanvas #54

wants to merge 2 commits into from

Conversation

Josephineoderland
Copy link

Copy link

@AntonellaMorittu AntonellaMorittu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Josephine, and big congratulations for completing the bootcamp and delivering your final project! 🎓
Praise for working alone and shipping an app in only 3 weeks, you should be proud! ⭐

The app is working even if not perfectly, and you were ambitious in shipping many different functionalities that will require more testing and refinement. Either ways, the code is really well structured and written, clean formatted and readable, the backend is robust and structured in modules to improve scalability and readability.
I think it's very cool you implemented a chat but I couldn’t test it as I have no friends.
You nailed responsiveness and accessibility.
Now here's some tips and things to consider to improve the app flow and UX:

  • avoid usage of the app if logged out - as half of it won’t work and the user is lost
  • please add a README for your app
  • please add app usage info. Overall the app is in need of UI info text and explanation on how to use the app.
  • avoid UI flickering of elements / re-render of the whole page (i.e. when you click "generate random characters")
  • search user to befriend, based on what criteria? Some info text would help the user understanding how to use the app in the best way
  • Please remove the console.log()s from the frontend used during developement

Generally it would have been better to focus on a simple MVP and simple functionalities well tested, rather than have extensive features and structure but weaker.

I'll ask you to apply the fixes mentioned, thank you and again big congratulations 👏 You made it 👩‍🎤

Comment on lines +32 to +34
<Route path="/CharacterGenerator" element={<CharacterGenerator />} />
<Route path="/WhoAndWhatG" element={<WhoAndWhatG />} />
<Route path="/PlaceAndFeelG" element={<PlaceAndFeelG />} />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be consistent with the naming convention, in url there should only be kebab-case and not PascalCase names 👍

@Josephineoderland
Copy link
Author

Hi,

I pushed up the changes as requested.
But I don't understand what you mean with the flickering? I have asked colleagues and they don't understand either. Please explain further what you ment or if the changes I have done is enough.

Copy link

@HIPPIEKICK HIPPIEKICK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Antonella that there's still room for UI improvements, as it might not be completely intuitive, but I think you've improved it and it reaches the requirements, so I'll approve now.

I'm not a 100% sure, but I think the flickering refers to this 👀
https://github.com/user-attachments/assets/bccf3610-da69-43d6-a114-2e6e76265581

@Josephineoderland
Copy link
Author

Josephineoderland commented Aug 5, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants