Skip to content

Conversation

@giangnht19
Copy link

@giangnht19 giangnht19 commented Sep 3, 2025

Description

This PR populates the Overlay component for the Detailed Unit view with the full content of the unit.
Previously, the overlay component was only a placeholder without meaningful content. With this change, when a user selects a unit, the overlay will display its details in a clear and structured format.

Note: The actual unit data is not yet populated because the backend requirement model is still pending. Once that backend feature is updated, the overlay will dynamically render the real unit content.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

The overlay component has been tested with mock data and placeholder content to ensure proper functionality.

  • Verified that selecting a unit correctly opens the overlay with the placeholder structure populated.
  • Confirmed that no runtime errors occur when rendering the overlay in the absence of backend data.
  • Checked responsiveness and layout consistency across browsers.
image

Testing Checklist:

  • Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from @macite and @jakerenzella on the Pull Request

Copy link

@Duy-Nguyen1104 Duy-Nguyen1104 left a comment

Choose a reason for hiding this comment

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

Hi, I've reviewed the PR and tested the new features. Here are some of my observations:

  • For the unit details overlay, the modal popup cleanly displays unit information and integrates well with the unit definition data.
  • Mark Unit as Complete: The functionality works as expected - completed units are properly locked (cannot be moved/removed), which maintains data integrity

However, regarding the Skill Summary section, I think it is redundant and should be removed. The current implementation shows the skill summary as an option for each individual unit, but each unit displays the same progress data (all completed units). This creates unnecessary redundancy and could confuse users.
=> Recommendation: Remove the per-unit skill summary option and instead create a dedicated progresss component (should be a separated PR). This would provide a better user experience by consolidating progress information in one centralized location.

Overall, this is good work. Well done!

Copy link

@ibi420 ibi420 left a comment

Choose a reason for hiding this comment

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

Hello, I’ve pulled and run this PR, and I can confirm it works as expected—the overlay correctly populates with the unit information. You’ve also successfully implemented Duy’s feedback, and this incorporation doesn’t break any functionality. Your code is well-written and well-commented. Great job completing this task, and thank you for the opportunity to review your work.

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