Finish implementing EntityUpload except for warnings #961
Conversation
matthew-white
changed the title
Also adding formatList().
| // no height to use. | ||
| let previousHeight = 0; | ||
| watch( | ||
| [() => props.entities, () => props.pageSize], |
There was a problem hiding this comment.
props.pageSize can change before props.entities, not just concurrently. After the user makes a selection, props.pageSize changes pretty much immediately, but for the first table, props.entities lags behind it: it only changes after the response. That's a problem, because changing props.pageSize will reset minHeight above, yet we don't want to do that while the request is in progress. We should probably watch just props.entities, not props.pageSize.
Let's make sure to add a test for this case.
There was a problem hiding this comment.
This issue is now being tracked at getodk/central#655.
| const resizeLastColumn = () => { | ||
| // Undo previous resizing. | ||
| const th = container.value.querySelector('th:last-child'); | ||
| th.style.width = ''; |
There was a problem hiding this comment.
I think there's an issue with the approach here:
- Use an entity list with enough properties that the tables scroll horizontally.
- Cause the file pop-up to overlap the second table, e.g., by increasing the page size of the first table. Doing so will cause the last column to widen.
- Scroll all the way to the right of the second table.
- Cause a change in the modal body that doesn't change the conditions above, e.g., go to the next page of a table.
- Observe that the scroll position of the second table has changed: it is no longer at the end (all the way to the right).
There was a problem hiding this comment.
This issue is now being tracked at getodk/central#655.
| ); | ||
|
|
||
| const overlapsPopup = ref(false); | ||
| const resizeLastColumn = () => { |
There was a problem hiding this comment.
I'm seeing an issue that may have to do with this function:
- Set up the modal such that when a file is selected, the pop-up will overlap the second table. For example, select a larger page size for the first table.
- (Not sure whether this matters:) Use an entity with fewer properties, such that the tables don't scroll horizontally, and the last column ends up being wider than the other columns.
- Select a file. Notice that a scrollbar appears below the second table. Once the animation ends, the scrollbar disappears again. A scrollbar should never appear.
- Maybe another variable is the width of the viewport?
Are we definitely waiting for the animation to end before calling this function?
- Is
popupAnimating.valueset beforeresizeColumnUnlessAnimating()is called? Is theanimationstartevent fired in time? - What if we don't listen for
animationstartand instead setpopupAnimating.valuetotrueat the end ofselectFile()?
There was a problem hiding this comment.
This issue can actually be seen in the forum announcement for v2024.1: https://forum.getodk.org/t/odk-central-v2024-1-add-entities-by-uploading-a-csv/46616. You can see that a scrollbar appears below the second table during the animation.
This issue is now being tracked at getodk/central#655.
|
I've pushed a commit that resolves some, but not all of the comments above. Most of the comments that I didn't resolve are a bit fiddly. I don't think they need to be resolved before regression testing begins. I'm going to go ahead and merge this PR, then follow up with another PR later to resolve the remaining comments. These are the comments that are currently remaining: |
matthew-white
changed the title
Closes getodk/central#589.
What has been done to verify that this works as intended?
This PR includes some changes to tests, but mostly I've just been doing manual testing locally. The QA team will also be taking a look at this PR. I'll be adding more tests as part of #979.
Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes