Confirmation Model with jQuery

Question

I have a link aimed to '/FridgeTemperatureRoute' and a checkbox. Purpose of the checkbox is to show the user whether the check has been done or not.

 <div class="row">
 <div class="col">
 <a id="FridgeTemperatureBtn" href="/FridgeTemperatureRoute">Fridge and Freezer Temperature</a>
 </div>
 <div class="col">
 <input id="FridgeTemperatureCheckBox" type="checkbox" name="" value="" disabled {{fridgeFreezerTemperatureReturnObj}}>
 </div>
 </div>

Summary of code

You'll see this line {{fridgeFreezerTemperatureReturnObj}} - this is from Node JS server, which determines whether the checkbox is ticked or not.

Jquery, Javascript and Bootstrap Model

Next, I am aiming to open a bootstrap model on user click, but only if the checkbox is ticked. If the user clicks and the checkbox is checked, then open a bootstrap model asking whether to continue to the href or simply cancel rendering the next page.

 <!-- Modal confirm -->
<div class="modal" id="confirmModal" style="display: none; z-index: 1050;">
 <div class="modal-dialog">
 <div class="modal-content">
 <div class="modal-body" id="confirmMessage">
 <p>You have already completed this check, do you want to do it again?</p>
 </div>
 <div class="modal-footer">
 <button type="button" class="btn btn-default" id="confirmOk">Ok</button>
 <button type="button" class="btn btn-default" id="confirmCancel" data-dismiss="modal">Cancel</button>
 </div>
 </div>
 </div>
</div>
<script type="text/javascript">
 if(document.getElementById('FridgeTemperatureCheckBox').checked){
 $('#FridgeTemperatureBtn').attr('data-toggle', 'modal');
 $('#FridgeTemperatureBtn').attr('data-target', '#confirmModal');
 document.getElementById('FridgeTemperatureBtn').addEventListener('click', function(e){
 $('#confirmOk').click(function(){
 window.location.href = '/FridgeTemperatureRoute';
 }); 
 });
 }
</script>

Summary

I have it working - but I believe it can be simplified. Just looking for some reviews and where I can improve on :)

Answer 1

Be consistent

If you're going to use jQuery, it would be stylistically appropriate (and terser) to use it instead of the native DOM methods. For example:

if(document.getElementById('FridgeTemperatureCheckBox').checked){

can turn into

if ($('#FridgeTemperatureCheckBox').is(':checked')) {

To translate getElementById to jQuery or querySelector, put a # in front of the ID you want to select.

Or you can use vanilla DOM methods everywhere if you prefer (like I do) - but whatever you choose to do, it would be good to do it consistently, otherwise you'll probably confuse future readers of the code.

HTML too: On a similar note about consistency - with your element IDs, better to choose either camelCase, PascalCase, or kebab-case everywhere, but don't mix and match different styles, else you're more likely to run into typo problems when you don't have a set style and mis-remember which style you used. (You can also consider using classes instead of IDs, since IDs with their automatically global variables are unintuitive and can cause bugs.)

DRY code

Rather than selecting the same element multiple times, eg:

$('#FridgeTemperatureBtn').attr('data-toggle', 'modal');
$('#FridgeTemperatureBtn').attr('data-target', '#confirmModal');

It would be more elegant to select the element once and save it in a variable. Or, even better, since .attr returns the existing jQuery collection, just chain the .attr calls:

$('#FridgeTemperatureBtn')
 .attr('data-toggle', 'modal')
 .attr('data-target', '#confirmModal');

Attach listener once, immediately: It looks like the confirmModal only becomes visible when the user clicks the button and has already completed the check, is that correct? If so, consider attaching the #confirmOk click listener immediately, rather than only after the #FridgeTemperatureBtn is clicked. (Attaching identical listeners multiple times won't cause problems in this case, but it's peculiar logic.)

Semantics: Your FridgeTemperatureBtn is an <a>, not a button, so its ID's name should probably be tweaked. If you have it styled to look like a button with your CSS, it would probably be a bit more semantically appropriate to surround a <button> inside an <a> instead.

Unchangeable checkbox UX: The #FridgeTemperatureCheckBox is disabled. Having it be a checkbox is a bit unintuitive, because users generally expect checkboxes to be clickable. Consider informing the user that the check has already been done some other way, such as by adding a checkmark (not a checkbox), or by changing the background color, or something like that.

• CertainPerformance: Yeah, I think you're right. Sectioning is a lot more suited for CR which is often about nitpicking a bunch of things.