Merged
Conversation
2 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the XML upload UX by adding a loading/disabled state during processing and switching success feedback from an in-modal alert to a Bootstrap toast, along with updated translations and tests.
Changes:
- Add upload “loading” state (spinner + disable file input/drop zone) and reset modal state on close.
- Add Bootstrap toast notifications for upload success/error feedback.
- Update i18n strings and expand unit/Playwright test coverage for the new UI behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
js/upload.js |
Adds loading-state toggling, toast feedback helper, and modal reset-on-close behavior. |
modals.html |
Adds spinner markup in upload modal and a global toast container for upload feedback. |
lang/en.json |
Adds new upload loading/toast strings. |
lang/de.json |
Adds new upload loading/toast strings. |
lang/fr.json |
Adds new upload loading/toast strings. |
tests/js/upload.module.test.js |
Adds module-level tests for loading state, toast behavior, and modal reset behavior. |
tests/js/upload.test.js |
Updates jsdom DOM fixtures and adds tests around loading/toast logic (currently via local re-implementations). |
tests/playwright/flows/xml-upload-flow.spec.ts |
Updates e2e assertions from in-modal status to toast-based success feedback. |
Comments suppressed due to low confidence (1)
tests/js/upload.test.js:83
- This test file re-implements production functions inline (e.g.,
showUploadStatus,handleXmlFile,setUploadLoadingState,showUploadToast) instead of importingjs/upload.js. That means it won’t catch regressions in the real implementation and can drift as the code evolves (especially now thatupload.jsexports these helpers). Consider switching these tests torequire('../../js/upload.js')and asserting against the exported functions, or removing this file in favor ofupload.module.test.jsif that’s the intended canonical coverage test.
describe('showUploadStatus', () => {
test('displays success message with correct class', () => {
function showUploadStatus(message, type) {
const statusElement = $('#xml-upload-status');
statusElement.removeClass()
.addClass(`alert alert-${type}`)
.removeClass('d-none')
.text(message);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/js/upload.module.test.js:136
- Several tests in this file call showUploadStatus() without enabling fake timers or cancelling the auto-hide timeout. Since showUploadStatus schedules a 10s setTimeout, these tests will leave real timers pending (making the Jest run slower and potentially causing open-handle/leak warnings). Consider using jest.useFakeTimers() for this describe block or calling uploadModule.clearStatusHideTimer() (or jest.runOnlyPendingTimers()) in test/afterEach cleanup.
describe('showUploadStatus', () => {
test('shows success message', () => {
uploadModule.showUploadStatus('Success message', 'success');
const statusElement = $('#xml-upload-status');
expect(statusElement.text()).toBe('Success message');
expect(statusElement.hasClass('alert-success')).toBe(true);
expect(statusElement.hasClass('d-none')).toBe(false);
});
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request enhances the XML upload experience by improving user feedback, accessibility, and error handling in the upload modal and related scripts. It introduces toast notifications for upload results, adds a loading spinner, internationalizes all user-facing messages, and refactors the upload logic for better maintainability and testability.
Changes
User Experience and UI Improvements
modals.html,js/upload.js) [1] [2].modals.html,js/upload.js) [1] [2].js/upload.js).Internationalization and Accessibility
js/upload.js,lang/en.json,lang/de.json,lang/fr.json) [1] [2] [3] [4].modals.html).Robustness and Refactoring
isXmlFileutility function for consistent file type checking and improved error feedback on invalid uploads (js/upload.js) [1] [2] [3].js/upload.js,tests/js/upload.test.js) [1] [2] [3].Testing and Maintenance
tests/js/upload.test.js) [1] [2].tests/js/authorInstitution.test.js,tests/js/saveHandler.test.js) [1] [2].Notes for Reviewer
None.
Checklist
Known Issues
None.