Migrate/comments-modal component

[b0ink]

Student ID: s222313935

Description

Migrates the comments-modal component from CoffeeScript to TypeScript. This component is used to display images and PDFs from the task comments as a fullscreen modal popup, allowing users to view the content without needing to download or open it in another tab.

Type of change

• Migration

How Has This Been Tested?

• Log in as student_1 and select any unit and any task.
• Send an image and a PDF into the task comments either by manually attaching the files using the bottom left icon or dragging and dropping it over.
• Once uploaded, confirm the comments-modal is working correctly by clicking on the attachments in the comments and view images in fullscreen or PDFs embedded into the modal.

Before (images):

After (images):

Before (PDFs):

After (PDFs):

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

Component review PR: thoth-tech/documentation#519

[AusBruce]

hi, there!

[b0ink]

I have requested for @AusBruce to review this PR

[atharv02-git]

Ontrack Component-Comments-Modal Migration review doubtfire-lms#519

Review by Team Member Atharv

Component Name: Comments Modal

Component Review:

Hi Eliya, firstly you've done a great job migrating the "comments-modal" component, so Kudos to that!!!
The code seems to be cleaner, and more maintainable. Honestly, I haven't cloned your repository, because of some issues with my local container setup but looking at the screenshot, the code seems to work absolutely fine.

So, if you could just click on this link you could see few suggestions I posted, so please have a look on that as well.
Thank you!

[AusBruce]

Review:

The replacement files are created successfully.
The old component is unlinked and new component is successfully added.
HTML, CoffeeScript, and SCSS are migrated successfully and no error
The website remain the same as picture shows
Great. It is good to have upstream review!

[satikaj]

Great work so far @b0ink

One thing I noticed when testing with an image, if it's too big it was scrollable in the original, and in the new version it just restricts it to the viewport size. Could you look into this?

Also, if possible, can you use Tailwind classes for the styles instead of SCSS

[b0ink]

One thing I noticed when testing with an image, if it's too big it was scrollable in the original, and in the new version it just restricts it to the viewport size. Could you look into this?

@satikaj Closest I could get to make it scrollable like the original from within the component's styling was adding scroll overflow on just the modal itself 8ff67d2 but if we wanted the same behaviour as bootstrap's modals where you can also scroll on the opaque backdrop we would have to add overflow to .cdk-global-overlay-wrapper globally - but this would break the click to close modal functionality

Also worth noting that the modal's box-shadow is lost due to this commit

[satikaj]

I see. Should be fine since this is the closest we can get it.

Please make an upstream PR to doubtfire-lms/doubtfire-web 9.x branch @b0ink

[b0ink]

Made one small change to use overflow-y-auto instead of overflow-y-scroll so that the blank scrollbar doesn't show up unless the image is oversized and becomes scrollable

PR has been opened: doubtfire-lms#869

[AusBruce]

Tested, if the image is oversize then will have a scrollbar.