Skip to content

Recommendations #24

Open
aokounev wants to merge 104 commits intoCode-the-Dream-School:mainfrom
aokounev:main
Open

Recommendations #24
aokounev wants to merge 104 commits intoCode-the-Dream-School:mainfrom
aokounev:main

Conversation

@aokounev
Copy link

Please review my code for further improvements

Copy link

Choose a reason for hiding this comment

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

These files are normally not committed to a project. You can ignore everything in the static directory by adding static/ to your .gitignore file. To stop tracking these files I think you can do git rm static

Copy link

Choose a reason for hiding this comment

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

this file can be deleted, only the package.json and package-lock.json files are needed

Copy link

Choose a reason for hiding this comment

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

Nice job on the search! Since the search is filtering results on the todo section rather than the navbar I'd suggest moving the search feature into the todo section, possibly here:
image

Copy link

Choose a reason for hiding this comment

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

The sorting is super slick, it's fast and easy. It might be easier for users to understand how the results are sorted, which field and which order, if you move the sort options into a dropdown select like this
image

This way the selected option will display and the user will know how the results are sorted. What do you think of this?

Copy link

Choose a reason for hiding this comment

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

And more thoughts on sort actually: I found I could sort the todos, then add a todo that was sorted onto a different page and so couldn't actually see the todo I just created.

I'm wondering if it's necessary for the user to be able to sort by different fields. What was your intention with this feature? One way to address the behavior I found is to remove the different sort options and to always sort by create time descending.

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