[Feature:InstructorUI] Move add section button#12483
Conversation
Copied style for icon from user-profile.css
- moved styling to server.css - fixed indentation of button - restored submit button
Also removed comment from server.css
…te-section-to-button
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12483 +/- ##
============================================
- Coverage 21.66% 21.65% -0.01%
- Complexity 9639 9643 +4
============================================
Files 268 268
Lines 36226 36235 +9
Branches 487 487
============================================
Hits 7847 7847
- Misses 27896 27905 +9
Partials 483 483
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@Rkoester47 Is this blocked by #12412? |
I was worried about that, so I made this branch off of the branch for #12412 instead of main in the hopes it would avoid issues. |
|
@Rkoester47 Please convert this to a draft until #12412 is merged to make it clear that this isn't ready to be merged right now. You can go ahead and find reviewers for this PR (hopefully the same reviewers you got for #12412) while it's a draft. |
Sounds good, I'll switch it back to ready once #12412 is merged. |
Eli-J-Schwartz
left a comment
There was a problem hiding this comment.
I performed a functionality review of this PR. The form successfully appeared when the button was pushed, and allowed for new sections to be created when both entries were filled out correctly.
However, I noticed a couple of issues:
- The form lacks details on the format of both the section ID and course ID. This can lead to confusion as to what these entries are and how they need to be filled out.
- The form requires a course ID to be filled out, even though it's labelled as optional. Attempting submit shows an error that the field needs to be filled.
- The form error ungracefully when the section ID is too long or has invalid characters, exiting but displaying an error message in the banner.
Now the form will make entering improper input into the section ID field impossible, which will handle user error more gracefully than letting the form close and show an error.
JManion32
left a comment
There was a problem hiding this comment.
Overall, the functionality is good, and I think a popup was the right choice here. I have some observations and changes to request:
Limiting the table to a percentage of its container looks good on large screens, but we need to keep more narrow widths in mind.
This requires either media queries, or, allowing the table to span the page (which I still don't think is a bad idea)
I get this console error whenever I complete the form:
Please also see my inline comments.
| The 'default section' is assigned to students who have self-registered for this course. <br> | ||
| Note: Sections can only be deleted if they contain no students. | ||
| </p> | ||
| <div> |
There was a problem hiding this comment.
Is this extra container necessary? It's a lot of extra diff.
| <div class="row"> | ||
| <div class="box col-md-6"> | ||
| <h2>Current Registration Section Counts</h2> | ||
| <div class="col-md-7 reg-section-container"> |
There was a problem hiding this comment.
This is adding unnecessary padding to the left of the table. It should be aligned with the content above it.
|
Additionally, I have a few comments that may not be related to this PR: I still think the delete column should have more space, especially when the count column has a lot of unused space: Can we also allow editing of the |
Eli-J-Schwartz
left a comment
There was a problem hiding this comment.
I did a functionality test, and all of the problems I brought up before have now been fixed. I see no major issues with the new pop-up interface. The only possible improvement I can see would not closing the pop-up menu when an already existing section ID is used, but considering a clear error message is displayed, I don't think it's necessary.
Table and header now wrap appropriately on smaller screens. Unwanted div removed, and table spans width of container again.
…/Submitty into move-add-section-button
JManion32
left a comment
There was a problem hiding this comment.
Code and functionality look good. Great work!
Why is this Change Important & Necessary?
Fixes #12429
Currently, the Manage Sections page has an "add sections" button awkwardly to the right of the sections table. This is partly due to previous PRs that have moved and removed neighboring features. After PR #12412 is merged, the space on the right will become more sparse.
Before:
What is the New Behavior?
Now, the Add Section functionality lives in a button above the table, which opens a popup form. This form allows a user to add a section, and optionally give that section a Course ID. The user can submit this form and see the changes update in the table.
After:
What steps should a reviewer take to reproduce or test the bug or new feature?
Login as instructor and navigate to the Manage Sections page. Test out the functionality of adding a section, with and without a course ID.
Automated Testing & Documentation
Other information
This is not a breaking change.