Fix modal vertical align

[tylerburleigh]

Description of the Pull Request (PR):

I noticed #instructions_modal was not displaying properly on smaller screens. It was not vertically center-aligned, and it had an awkward scrollbar.

To fix this, I removed margin-top: 200px from the modal and added a function to dynamically set margin-top based on window.height().

This fixes or addresses the following GitHub issues:

• Ref: #

Checkoff for all PRs:

• I have read the Guidelines for Contributing and this PR conforms to the stated requirements.
• I have updated the CHANGELOG

Attn: @expfactory-admin

[vsoch]

hey @tylerburleigh ! This is a great idea, but I don't want to add an entire new javascript library dependency just to tweak a style. Can you figure out an adjustment that doesn't require this? Meaning, css only, so you can add a class to define the modal, and then customize it (and possibly set a custom value for resolution less than some size).

[tylerburleigh]

I think the best CSS-only solution is to simply remove the margin-top property and leave it at that, but then the modal is top-aligned rather than vertically center-aligned. I considered media queries, but there is no elegant solution, that I'm aware of, that will work at all resolutions.

What's the issue with adding a js dependency? Would including the JS code inside each of the start.html, finish.html, and entry.html files be a better solution? If you are concerned about it mucking with other elements, then I could modify the JS code to target that specific modal's DOM ID.

[vsoch]

Take a look at libraries that define css properties based on the max-width, media (max width) eg see here:

https://stackoverflow.com/questions/21075983/how-to-use-particular-css-styles-based-on-screen-size-device

And for css sizes, you can also do percentages, so to respond to your statement directly:

I think the best CSS-only solution is to simply remove the margin-top property and leave it at that, but then the modal is top-aligned rather than vertically center-aligned.

Yeah, I prefer the centered look than top aligned!

I considered media queries, but there is no elegant solution that will work at all resolutions.

Most of the time I've seen these, there are several options. I would say having media queries for at least a few of the most likely resolutions (and maybe one imperfect) is better than having (more than one?) imperfect as you are seeing.

What's the issue with adding a js dependency?

It's generally best practice to keep dependencies to a minimal. It's also better practice to not add unnecessary javascript - it ultimately leads to an overly engineered thing. For essential functions (jquery) or established libraries (bootstrap) it's important to include, and when there are changes to the library (let's say some day we decide to do the work to update bootstrap) you wouldn't want to have many custom tweaks to have to redo. This is the same rationale for any software library - you install a particular version and don't then customize it on your server. Thus, for these larger / important and established libraries, I would include. But for trivial things like changing the position of a modal? You don't need javascript for that. Adding it might be easier, but it is not the right approach for long term health of the codebase.

Would including the JS code inside each of the start.html, finish.html, and entry.html files be a better solution?

It would be less ideal, because then you are copy pasting the same thing.

If you are concerned about it mucking with other elements, then I could modify the JS code to target that specific modal's DOM ID.

Nope, definitely not! I really like this suggestion to work on the style and I'd like to help you, and I think we should give media queries a try. Do you want to take a shot?

[tylerburleigh]

Alright, I found a CSS-based solution that doesn't use media queries so it can adapt to any viewport height. It involves overriding a few of bootstrap's CSS properties for .modal-dialog.