This PR makes progress on getodk/central#589, making EntityUpload a full-screen modal.
Why is this the best possible solution? Were any other approaches considered?
The main difficulty here was positioning .modal-actions. In a full-screen modal, .modal-actions should be positioned at the bottom of the modal. However, it's not easy to do that given that there aren't guarantees around the viewport height. It's possible for the modal to be taller than the viewport, so we can't simply set position: fixed; on .modal-actions. We could set position: absolute;, but that would lose the margin above .modal-actions. I ended up setting position: absolute; conditionally, in the case that the modal isn't taller than the viewport. If the modal is taller than the viewport, then .modal-actions will already end up at the bottom of the modal with the correct top margin; if the modal isn't taller than the viewport, then setting position: absolute; will only increase the space above .modal-actions. The downside of this approach is that setting position: absolute; conditionally requires JavaScript, and the condition needs to be reevaluated whenever the height of the modal changes. There is already JavaScript homework that the component needs to do when the height of the modal changes, so we're just adding to that, but it is additional code.
I tried to come up with an approach that wouldn't require JavaScript. I thought about increasing the bottom padding of a full-screen modal to include the height of .modal-actions and the margin above it. However, I wasn't sure that I should assume that the modal has a .modal-actions element, and if it doesn't have one, we wouldn't want to increase the bottom padding. I was also reluctant to make assumptions about the height of .modal-actions, since that could vary based on the font. We could add this padding dynamically using JavaScript, but that doesn't seem much better than the current approach.
I also thought about requiring the parent component of Modal (here, EntityUpload) to set sufficient bottom margin on the element above .modal-actions. However, it feels like this really should be a concern of Modal. Offloading it to the parent component would require exporting a variable for the top margin of .modal-actions. I think it's nicer for the parent component of a full-screen modal to be able to use .modal-actions normally, without special requirements.
What has been done to verify that this works as intended?
I viewed this change locally to make sure that the modal looked right, both when its content overflows the viewport and when it doesn't. I also tried out a couple of cases locally where position: absolute; is toggled on .modal-actions. I tried changing the content of the modal, and I tried resizing the window.
We generally don't test CSS, but I added tests of the classes involved in this change (that .modal-full is added to .modal-dialog and that .has-scroll is conditionally added to .modal). However, I didn't add tests for all the cases in which .has-scroll is toggled: that felt like more effort than it was worth.
Before submitting this PR, please make sure you have:
[x] run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
[x] verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced
This PR makes progress on getodk/central#589, making
EntityUploada full-screen modal.Why is this the best possible solution? Were any other approaches considered?
The main difficulty here was positioning
.modal-actions. In a full-screen modal,.modal-actionsshould be positioned at the bottom of the modal. However, it's not easy to do that given that there aren't guarantees around the viewport height. It's possible for the modal to be taller than the viewport, so we can't simply setposition: fixed;on.modal-actions. We could setposition: absolute;, but that would lose the margin above.modal-actions. I ended up settingposition: absolute;conditionally, in the case that the modal isn't taller than the viewport. If the modal is taller than the viewport, then.modal-actionswill already end up at the bottom of the modal with the correct top margin; if the modal isn't taller than the viewport, then settingposition: absolute;will only increase the space above.modal-actions. The downside of this approach is that settingposition: absolute;conditionally requires JavaScript, and the condition needs to be reevaluated whenever the height of the modal changes. There is already JavaScript homework that the component needs to do when the height of the modal changes, so we're just adding to that, but it is additional code.I tried to come up with an approach that wouldn't require JavaScript. I thought about increasing the bottom padding of a full-screen modal to include the height of
.modal-actionsand the margin above it. However, I wasn't sure that I should assume that the modal has a.modal-actionselement, and if it doesn't have one, we wouldn't want to increase the bottom padding. I was also reluctant to make assumptions about the height of.modal-actions, since that could vary based on the font. We could add this padding dynamically using JavaScript, but that doesn't seem much better than the current approach.I also thought about requiring the parent component of
Modal(here,EntityUpload) to set sufficient bottom margin on the element above.modal-actions. However, it feels like this really should be a concern ofModal. Offloading it to the parent component would require exporting a variable for the top margin of.modal-actions. I think it's nicer for the parent component of a full-screen modal to be able to use.modal-actionsnormally, without special requirements.What has been done to verify that this works as intended?
I viewed this change locally to make sure that the modal looked right, both when its content overflows the viewport and when it doesn't. I also tried out a couple of cases locally where
position: absolute;is toggled on.modal-actions. I tried changing the content of the modal, and I tried resizing the window.We generally don't test CSS, but I added tests of the classes involved in this change (that
.modal-fullis added to.modal-dialogand that.has-scrollis conditionally added to.modal). However, I didn't add tests for all the cases in which.has-scrollis toggled: that felt like more effort than it was worth.Before submitting this PR, please make sure you have:
npm run testandnpm run lintand confirmed all checks still pass OR confirm CircleCI build passes