Skip to content

Week 04 work#8

Merged
aslavchev merged 5 commits intomainfrom
week-04-work
Nov 27, 2025
Merged

Week 04 work#8
aslavchev merged 5 commits intomainfrom
week-04-work

Conversation

@kamenAsenov
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Owner

@aslavchev aslavchev left a comment

Choose a reason for hiding this comment

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

Week 4 PR Review - Request Changes

⚠️ CHANGES REQUESTED - 65/100

Hi Kamen, I can see you put effort into Week 4, especially in the usability and accessibility exercises. However, a few exercises appear incomplete and need some additional work before we can merge. Let me walk you through what needs attention.


Items That Need Completion

1. Security Checklist - Needs Testing

I noticed all the checkboxes are still empty in security-checklist.md. The goal of this exercise is to actually test each security item and mark what passes/fails.

Next steps:

  • Go through each item in the checklist and test it
  • Mark items with [x] if they pass
  • Add notes for any items that fail or behave unexpectedly

2. Compatibility Matrix - Missing Desktop Results

The mobile testing looks good with detailed observations! However, the desktop browser rows need the same level of detail:

Current:

  • Chrome Desktop: "Seems the same as Safari Desktop"
  • Safari Desktop: "All things tested until now are on Safari Desktop"

What I need:

  • Actually test each browser on desktop
  • Fill in the Login, Product Catalog, Shopping Cart, and Checkout columns
  • Document what you observe, just like you did for mobile

3. Small Performance Typo

In performance-assessment.md line 78: "37 s" should probably be "37 ms" (to match your other measurements)


4. Minor Workflow Items

A few housekeeping things:

  • Please remove .gitkeep from the week-04 folder (it's just a placeholder when folders are empty)
  • Assign the PR to me (@aslavchev) so I get notified
  • I noticed some Week 1-2 files got added and then removed in the commit history - might be worth double-checking which branch you're on before committing

What's Working Well ✨

  • Usability Evaluation: Really strong work here! You identified real UX issues like the quantity change problem and tax visibility. This shows good critical thinking.
  • Accessibility Audit: Great job testing with keyboard navigation and VoiceOver. Your severity ratings are appropriate.
  • Performance Assessment: Nice data collection and the metrics legend is helpful.

Next Steps

Once you've:

  • Completed the security testing
  • Filled in the desktop compatibility results
  • Fixed the typo
  • Removed .gitkeep
  • Assigned the PR to me

Let me know and I'll give it another review!

Note: I know you can deliver quality work - Week 3 showed that clearly. Take a bit of extra time to review your deliverables before submitting to catch these kinds of gaps.

Expected grade after updates: 75-80/100

- Alex

Copy link
Copy Markdown
Owner

@aslavchev aslavchev left a comment

Choose a reason for hiding this comment

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

Week 4 PR Review - Approved

✅ APPROVED - 78/100 (GOOD)

Great improvements, Kamen! You addressed all the critical issues.


What You Fixed ✅

  • Security Checklist: All items tested and checked - you even found a bug (empty cart checkout)!
  • Compatibility Matrix: Desktop browsers now have proper detailed results
  • Performance Typo: 37s → 37ms fixed

Content Quality

Strong work:

  • Security testing found real issues (empty cart bug, validation problems)
  • Compatibility matrix is thorough and consistent
  • Usability & accessibility evaluations were excellent

Minor Notes for Next Time

  • Remove .gitkeep when you add files to a folder
  • Assign PRs to mentor (@aslavchev) not yourself

These are small workflow items, not blocking.


Final Grade: 78/100 (GOOD)

Well done on the fixes. Your testing is improving - keep that critical thinking!

Status: Approved & Ready to Merge

- Alex

@aslavchev aslavchev merged commit 995d065 into main Nov 27, 2025
@aslavchev aslavchev deleted the week-04-work branch November 27, 2025 11:38
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