Skip to content

code review from zhangzhang li#3

Open
gloria20070108 wants to merge 1 commit intoMARVELOUSbear:mainfrom
gloria20070108:main
Open

code review from zhangzhang li#3
gloria20070108 wants to merge 1 commit intoMARVELOUSbear:mainfrom
gloria20070108:main

Conversation

@gloria20070108
Copy link

UX Feedbacks:

  1. Great Styles and the webpage is very easy to use!
  2. The "Tada" Dialog is cute. However, it is a little distractive for user.
  3. While filtering by a tag, there is no way to remove the filter except clicking the Home link. (this could be a future feature)
  4. The search item input requires enter to take effect. It will be better if it can do search with the input.
  5. the layout of Edit/Delete buttons in the "My Articles" may need some improve.
  6. Maybe have a logo to replace the "Your Company Logo" in the login page.

Code feedback:

  1. nice code organization!
  2. remove unused files like App.css.
  3. should have propTypes for all the components
  4. in NewArticle.js, used then() in some async functions. async/await should be better
  5. in myDB.js, "tagFilter === '' || tagFilter === undefined" could be simplied as "!tagFilter"

@chris19960730
Copy link
Collaborator

Hi Zhangzhang, thanks for the code review and your suggestions. Those will definitely help our app improve a lot.

For your feedback 3, we have propTypes check for all controlled components, I don't think it's necessary to add propTypes to uncontrolled ones. Besides, for feedback 5, our implementation is kinda hacky in that we only don't allow 'undefined' but 'null' is accepted, so we can't use the simpler one as '!tagFilter' though. Please let us know if we have anything to improve :)

@gloria20070108
Copy link
Author

Hello Christ, for feedback 3, I believe that it is the professor's requirement.

@chris19960730
Copy link
Collaborator

Yeah, I know. As I said we have propTypes check for all controlled components. Those uncontrolled ones, since they don't accept props at all, we can't define propTypes rather than have an empty object. Thank you anyway

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.

2 participants