[#12288] Admin notifications page: Fix notifications table overflow

[yujioshiro]

Fixes #12288

Outline of Solution

Issue: At small screen widths, the notifications table would become infinitely smaller and the contents inside would start to break.

Fix: I updated the css so that the notifications table's min-width: 650px and the div that the table is inside has overflow-x: scroll ensuring the table never shrinks less than 650px and the user can scroll horizontally to receive all information in the notification.

Here is a demo of the current behavior:

Here is the fixed behavior:

Here is the fixed behavior with multiple notifications:

[yujioshiro]

Looking at these changes a day later, I think there is a better way to write this.

If I'm understanding the code correctly, the SCSS file I edited should only style the notifications-table component, and even though the selector I wrote targets only the .col-12 class inside the notifications-table.comoponents.html, I think future developers would benefit from the selector being more specific. Two ways I can do this:

1. Adding an id to the container div
   Inside notifications-table.components.html file, I would add id="notifications-table-container" to <div class="col-12"> so that I can target #notifications-table-container inside the SCSS file instead of targeting col-12. This makes my intentions more specific and easier to understand.

2. Using the :has() selector
   Writing .col-12:has(#notifications-table) makes the selection more specific. This method changes only the SCSS file, but it's not supported by Firefox by default.

According to the documents, I think using the first method is more inline with how the project is written. I am interested to hear/read how others might feel!

[weiquu]

Hi @yujioshiro, thanks for the good research! I also prefer the first method, and I think you're right that it's more consistent with the rest of the project when compared to the second method.

Could you also check how other tables deal with the overflow / horizontal scrolling issue? In particular, I think some of them apply a table-responsive class to the parent div of the table. Not too sure if that works here as well, but if it does, I think it's better to use that for consistency.

[yujioshiro]

@weiquu Thank you! I looked at the behavior of other tables and you were right.

I added the table-responsive class to the parent div of the table and removed the col-12 selector.

[weiquu]

Hi @yujioshiro, thanks for the change! Do remember to update the snapshot tests. You can update the snapshots by running npm run test and pressing a to run all test cases. After that, check through the snapshots to make sure the changes are as expected, then press u to update them. You can find more details on snapshot testing here.

[yujioshiro]

@weiquu Thank you! I saw some checks fail and was reading through the snapshot test documentation but wasn't too sure what to do. I've read it further and watched a couple videos and think I understand it more now, but I just want to confirm...

When I check my snapshots to see if the update was done correctly, I check the .snap file and see if the changes I made in the .html are reflected exactly in that .snap file?

And if it's correct, I can push the changes in the .snap file to my remote repository?

[weiquu]

Hi @yujioshiro, I think your understanding is about right. For the checking of the update, when the snapshot test fails the first time, you should be able to see the differences between the snapshot file and the generated snapshot. Once you've verified that the differences are to be expected, you can press u to update the snapshot file to the generated snapshot. After that is done, the snapshot tests have been updated and you can push the changes.

You can of course check the .snap file as well afterwards, but the process above should work just fine. Do note that you should not be manually editing the .snap files (i.e. please use the auto-update mode as detailed above).

[yujioshiro]

@weiquu Great, I redid the snapshot test and was able to see where it failed. I confirmed the only difference between the old and newly generated .snap file are what I added into the .html component earlier. I have then pushed the changes and it looks like all checks have passed. Also want to confirm that I didn't manually edit the .snap file.

I also made sure to pull from the master branch before pushing my changes. I'm not sure if I missed something or it's because another branch was pulled into the master while the checks were being processed, but it looks like my branch is now out of date. What would be the best possible course of action?

[weiquu]

Hi @yujioshiro, thanks for the changes! I think some PRs were recently merged into the main branch, could you try pulling from it again?

[yujioshiro]

@weiquu Sure! I followed number 2 of step 3 of the process section of the documentation. Specifically, I used Option 1 (merge) to merge my branch and the newly advanced main branch.

I should have asked this before I pushed my changes, but when it asks for a commit message for the merge, should I be writing a reason for the merge every time or is the default commit message good enough?

[weiquu]

Hi @yujioshiro, just the default commit message is fine! Thanks for asking (:

[weiquu]

LGTM!

[yujioshiro]

@weiquu LET'S GOOOOO! Thank you so much for all your help! Learned a lot.

I looked at other PRs that have been closed recently and it looks like there is nothing else I need to do. But I just want to confirm that we just wait for this merge to be finalized (by someone with the correct perms) and then it's fully done, right?

[zhaojj2209]

LGTM