Open davidhorak opened 5 months ago
Hi @davidhorak
Thank you so much for your feedback! I have made some recent commits thanks to your comments.
First, please note there are new environment variables and a new migration to run.
I will address each of your comments below in order.
Accessibility: Could we apply some beset practices for accessibility, e.g. title for actionable elements.
I have added basic accessibility features in this commit, although there is room to continue work on this: https://github.com/treestompz/member_manager/commit/a63b2178fd0fd95738c754350d91c254a4ec0ec2
The listing page does not differentiate between empty list and empty search results, could we communicate this better to users?
Now showing different text between empty list and empty search results: https://github.com/treestompz/member_manager/commit/a63b2178fd0fd95738c754350d91c254a4ec0ec2#diff-3b64d8f2c478822af1eed609fba19ec12dac7b704cb0212e8e433fd2e8765248R68
The search is performed on multiple fields, how could we communicate this to the users?
Now using new placeholder text: https://github.com/treestompz/member_manager/commit/a63b2178fd0fd95738c754350d91c254a4ec0ec2#diff-3b64d8f2c478822af1eed609fba19ec12dac7b704cb0212e8e433fd2e8765248R23
How we would handle a long list of users on the listing page?
I have added a complete pagination system that works in the default view or search view. Note that team members are also now order by last name, first name: https://github.com/treestompz/member_manager/commit/3a6cacb51719662989d96497aaf95a2084201f1a
Having a lot of search results, how we could communicate the search hits to users on the individual cards?
member.first_name|highlight:request.GET.search
, we have to be extra careful to avoid XSS attacks. Given that the value of the feature is low (the user is only searching by 3 fields - it should be fairly obvious why a result was returned already) but the risk of disaster is high (XSS attacks,) I’ll choose to backlog this feature for now. In a real scenario, this is something we would want to take our time with and also get properly peer-reviewed.
I was able to delete all users. Let's assume that only admins would be able to add users, how we could setup the app to prevent a dead-end situation?
- I’m not entirely sure I understand what you mean here. Can you please clarify?
- Are you talking about a scenario where there must be at least 1 admin user in the app? If so, we can add that logic to the code where appropriate, and might want to also consider bootstrapping a default user if no admins users exist.
Navigation: not mapped urls give me an a hard 404 server error
I have added custom templates that match our app's style for 400, 403, 404, and 500 errors: https://github.com/treestompz/member_manager/commit/a63b2178fd0fd95738c754350d91c254a4ec0ec2#diff-3ce6e17f19c5df753b29263cd246d6dff65d6ab8eec278f97931f35d06e51ec3
Tests: it would great to validate some of the functionality via tests
Agreed 100%. Tests are something I would definitely add for a real production application, but given I have upcoming Instawork interviews I want to prepare for, I will not be adding them here.
Thanks again for your feedback and great suggestions. I understand this is an interview exercise so please don't feel any pressure to continue - I'm sure you're quite busy. However, if you want to, I more than welcome it!
Take care, Stefan
@treestompz Thank you spending an extra time to address the feedback. No need to spent any extra time, I believe that we have now a good baseline here! Great job! 👏🏻
@davidhorak Thanks David - much appreciated! 😄
Hi Stefan! Thank you for your submission, below are some notes.
Good
Areas of improvements
Kind regards, David (Instawork)