Skip to content

[Refactor:Notifications] Replace toasts with vue#11857

Merged
bmcutler merged 7 commits intomainfrom
toast-vue
Jul 16, 2025
Merged

[Refactor:Notifications] Replace toasts with vue#11857
bmcutler merged 7 commits intomainfrom
toast-vue

Conversation

@lavalleeale
Copy link
Copy Markdown
Contributor

@lavalleeale lavalleeale commented Jul 7, 2025

Why is this Change Important & Necessary?

As we are moving to vue this is a good early component since there are many places on the site that make toasts manually which seem to have drifted to cause certain toasts to have different html.

What is the New Behavior?

All toasts are created in vue.

What steps should a reviewer take to reproduce or test the bug or new feature?

  1. Go to main
  2. Look at toasts (login/logout/submission)
  3. Switch to branch
  4. Look at toasts (login/logout/submission)
  5. Functionality should be unchanged

Automated Testing & Documentation

Other information

@github-project-automation github-project-automation bot moved this to Seeking Reviewer in Submitty Development Jul 7, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 20.70%. Comparing base (e514c58) to head (ff245e6).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main   #11857   +/-   ##
=========================================
  Coverage     20.69%   20.70%           
  Complexity     9197     9197           
=========================================
  Files           265      265           
  Lines         35234    35222   -12     
  Branches        458      456    -2     
=========================================
- Hits           7293     7292    -1     
+ Misses        27487    27478    -9     
+ Partials        454      452    -2
Flag Coverage Δ
autograder 21.34% <ø> (ø)
js 2.11% <0.00%> (-0.03%) ⬇️
migrator 100.00% <ø> (ø)
php 19.37% <ø> (ø)
python_submitty_utils 80.08% <ø> (ø)
submitty_daemon_jobs 88.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martig7 martig7 self-requested a review July 8, 2025 16:26
@automateprojectmangement automateprojectmangement bot moved this from Seeking Reviewer to In Review in Submitty Development Jul 8, 2025
martig7
martig7 requested changes Jul 8, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@martig7 martig7 left a comment

Choose a reason for hiding this comment

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

Image

The spacing here is bad.

@github-project-automation github-project-automation bot moved this from In Review to Work in Progress in Submitty Development Jul 8, 2025
@martig7
Copy link
Copy Markdown
Contributor

martig7 commented Jul 8, 2025

image
This can be fixed by giving the icon more padding. I'll check what it looks like on main, but either way it's bad.

@lavalleeale lavalleeale requested a review from martig7 July 8, 2025 20:05
@automateprojectmangement automateprojectmangement bot moved this from Work in Progress to In Review in Submitty Development Jul 8, 2025
JManion32
JManion32 reviewed Jul 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@JManion32 JManion32 left a comment

Choose a reason for hiding this comment

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

Functionality and code looks good. The right margin of the status icon is now apparent and looks much better.

Just change the name of toasts.vue to Toasts.vue and I will approve.

JManion32
JManion32 approved these changes Jul 14, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@JManion32 JManion32 left a comment

Choose a reason for hiding this comment

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

Functionality and Code looks good

@bmcutler
Copy link
Copy Markdown
Member

bmcutler commented Jul 15, 2025

@lavalleeale Please fix merge conflicts

martig7
martig7 approved these changes Jul 15, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@martig7 martig7 left a comment

Choose a reason for hiding this comment

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

This refactor looks good, the toast is formatted correctly now.

@github-project-automation github-project-automation bot moved this from In Review to Awaiting Maintainer Review in Submitty Development Jul 15, 2025
@bmcutler bmcutler merged commit 7bc444c into main Jul 16, 2025
25 checks passed
@bmcutler bmcutler deleted the toast-vue branch July 16, 2025 15:34
williamschen23 pushed a commit that referenced this pull request Jul 18, 2025
@lavalleeale @williamschen23
[Refactor:Notifications] Replace toasts with vue (#11857)
bc25087 
### Why is this Change Important & Necessary?
<!-- Include any GitHub issue that is fixed/closed using "Fixes
#<number>" or "Closes #<number>" syntax.
Alternately write "Partially addresses #<number>" or "Related to
#<number>" as appropriate. -->
As we are moving to vue this is a good early component since there are
many places on the site that make toasts manually which seem to have
drifted to cause certain toasts to have different html.

### What is the New Behavior?
<!-- Include before & after screenshots/videos if the user interface has
changed. -->
All toasts are created in vue.

### What steps should a reviewer take to reproduce or test the bug or
new feature?
1. Go to main
2. Look at toasts (login/logout/submission)
3. Switch to branch
4. Look at toasts (login/logout/submission)
5. Functionality should be unchanged

### Automated Testing & Documentation
<!-- Is this feature sufficiently tested by unit tests and end-to-end
tests?
If this PR does not add/update the necessary automated tests, write a
new GitHub issue and link it below.
Is this feature sufficiently documented on submitty.org?
Link related PRs or new GitHub issue to update documentation. -->

### Other information
<!-- Is this a breaking change?  
Does this PR include migrations to update existing installations?  
Are there security concerns with this PR? -->
@williamschen23 williamschen23 mentioned this pull request Aug 15, 2025
Merged
bmcutler pushed a commit that referenced this pull request Aug 18, 2025
@williamschen23
[Bugfix:InstructorUI] Bulk upload toast fix (#12009)
3d42bd3 
### Why is this Change Important & Necessary?
<!-- Include any GitHub issue that is fixed/closed using "Fixes
#<number>" or "Closes #<number>" syntax.
Alternately write "Partially addresses #<number>" or "Related to
#<number>" as appropriate. -->
#11857, we replace all the toasts with vue. however, there was still
some places where jquery calls `#messages` which could desync the vue
files content. This causes a very weird error,
```
    TypeError: Cannot read properties of null (reading 'insertBefore')
    at insert (vue.runtime.global.prod.js?v=1755186796:6:53704)
    at O (vue.runtime.global.prod.js?v=1755186796:6:10185)
    at R (vue.runtime.global.prod.js?v=1755186796:6:9733)
    at C (vue.runtime.global.prod.js?v=1755186796:6:9199)
    at Y (vue.runtime.global.prod.js?v=1755186796:6:18714)
    at G (vue.runtime.global.prod.js?v=1755186796:6:17988)
    at U (vue.runtime.global.prod.js?v=1755186796:6:12056)
    at C (vue.runtime.global.prod.js?v=1755186796:6:9160)
    at F (vue.runtime.global.prod.js?v=1755186796:6:11493)
    at D (vue.runtime.global.prod.js?v=1755186796:6:10902)
```
which does not really make sense. This is because of the desync between
js and vue.

### What is the New Behavior?
<!-- Include before & after screenshots/videos if the user interface has
changed. -->
This removes allof the references to `#messages`, which causes the bulk
upload page to have the notifications.

### What steps should a reviewer take to reproduce or test the bug or
new feature?
Make an error on the page, like entering a page count that is too large,
and click bulk upload. Nothing should show up, but there should be
notifications after viewing this PR

### Automated Testing & Documentation
<!-- Is this feature sufficiently tested by unit tests and end-to-end
tests?
If this PR does not add/update the necessary automated tests, write a
new GitHub issue and link it below.
Is this feature sufficiently documented on submitty.org?
Link related PRs or new GitHub issue to update documentation. -->

### Other information
<!-- Is this a breaking change?  
Does this PR include migrations to update existing installations?  
Are there security concerns with this PR? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@martig7 martig7 martig7 approved these changes

@JManion32 JManion32 JManion32 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Submitty Development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lavalleeale @martig7 @bmcutler @JManion32