patternjavascriptMinor
Constructor function, prototype and making a modal
Viewed 0 times
modalfunctionconstructorprototypeandmaking
Problem
I'm playing around with prototypes, constructor functions and javascript - I'm not really a Javascript developer but am interested in this.
Am I on the right track with this code? I'm looking for a code review from a JS developer. I know the codes a bit funky.
I created an example of it working here.
`function modal( width, height, backgroundColor, overlayColor, showOverlay, url )
{
this.width = width; // a percentage based number %
this.height = height; // optional, usually needs not to be set
this.backgroundColor = backgroundColor; // set the background colour to the modal
this.overlayColor = overlayColor; // as hex, to be converted to rgba
this.url = url;
this.showOverlay = showOverlay;
this.createModal = function()
{
// create elements to add classes to and insert in to dom
var modalOverlay = document.createElement('div')
, modalContent = document.createElement('div')
, modalInner = document.createElement('div');
// build html structure
modalOverlay.className = "modalOverlay";
modalContent.className = "modalContent";
modalOverlay.appendChild(modalContent);
modalInner.className = "modalInner";
modalContent.appendChild(modalInner);
// add new html structure to body
document.body.appendChild(modalOverlay);
// Theme model
this.themeModel();
}
this.themeModel = function() {
// is there a background color?
var overlayBgColor = this.showOverlay ? this.convertHext(this.overlayColor,50) : "transparent";
$('.modalOverlay').css({
'background-color': overlayBgColor
});
$('.modalContent').css({
'background-color': this.backgroundColor,
'width': this.width,
'height': this.height
});
// load content
this.loadContent();
}
this.loadContent = function(){
if( url )
{
Am I on the right track with this code? I'm looking for a code review from a JS developer. I know the codes a bit funky.
I created an example of it working here.
`function modal( width, height, backgroundColor, overlayColor, showOverlay, url )
{
this.width = width; // a percentage based number %
this.height = height; // optional, usually needs not to be set
this.backgroundColor = backgroundColor; // set the background colour to the modal
this.overlayColor = overlayColor; // as hex, to be converted to rgba
this.url = url;
this.showOverlay = showOverlay;
this.createModal = function()
{
// create elements to add classes to and insert in to dom
var modalOverlay = document.createElement('div')
, modalContent = document.createElement('div')
, modalInner = document.createElement('div');
// build html structure
modalOverlay.className = "modalOverlay";
modalContent.className = "modalContent";
modalOverlay.appendChild(modalContent);
modalInner.className = "modalInner";
modalContent.appendChild(modalInner);
// add new html structure to body
document.body.appendChild(modalOverlay);
// Theme model
this.themeModel();
}
this.themeModel = function() {
// is there a background color?
var overlayBgColor = this.showOverlay ? this.convertHext(this.overlayColor,50) : "transparent";
$('.modalOverlay').css({
'background-color': overlayBgColor
});
$('.modalContent').css({
'background-color': this.backgroundColor,
'width': this.width,
'height': this.height
});
// load content
this.loadContent();
}
this.loadContent = function(){
if( url )
{
Solution
Here's some thoughts
One problem some programmers rant with this is that if you forget a parameter, or forget the order. This would seriously impale your code. An alternative is to pass an object that contains the config.
One advantage of this over the previous is that you become more verbose and don't need to remember the order of the params. Additionally, you can use
You lose the advantage of prototypal inheritance. Better put methods on the prototype so they are shared. Advantage is you save memory by not duplicating functions per instance. Disadvantage is you lose emulated private property - but everything is accessible one way or another, no point using privates.
Suggesting you move this to CSS by adding/removing classes. That way, proper separation of concerns is attained. You don't want to be debugging and 1 hour later, realize that the styles were not in the CSS files but in JS files.
To clarify, this kind of code will bite back in the long run. Suggesting you add a class to the top-most element of your modal HTML, and then style relative to it. You can provide a custom class name to the constructor instead of the styles.
IMO,
String cocatenation in JS is really messy with all that quotes. Here's an alternative:
Suggesting you look at how Bootstrap implemented their modal. It's in a form of a jQuery plugin. Your code looks cleaner if it did something as clean as:
function modal(width, height, backgroundColor, overlayColor, showOverlay, url) {One problem some programmers rant with this is that if you forget a parameter, or forget the order. This would seriously impale your code. An alternative is to pass an object that contains the config.
new Modal({
width : 100,
height : 100
});One advantage of this over the previous is that you become more verbose and don't need to remember the order of the params. Additionally, you can use
$.extend to place in defaults, if they didn't exist on options.function Modal(options){
options = $.extend(options,{
width : /*default width */,
height : /*default height */
});
...
}this.createModal = function () {...};
this.themeModel = function () {...};You lose the advantage of prototypal inheritance. Better put methods on the prototype so they are shared. Advantage is you save memory by not duplicating functions per instance. Disadvantage is you lose emulated private property - but everything is accessible one way or another, no point using privates.
$('.modalOverlay').css({
'background-color': overlayBgColor
});
$('.modalContent').css({
'background-color': this.backgroundColor,
'width': this.width,
'height': this.height
});Suggesting you move this to CSS by adding/removing classes. That way, proper separation of concerns is attained. You don't want to be debugging and 1 hour later, realize that the styles were not in the CSS files but in JS files.
To clarify, this kind of code will bite back in the long run. Suggesting you add a class to the top-most element of your modal HTML, and then style relative to it. You can provide a custom class name to the constructor instead of the styles.
.blackish-modal a{/*style for links on modal with blackish-modal class*/}
.whitish-modal a{/*same goes, but for whitish-modal class*/}$('.modalInner').load(url);IMO,
$.fn.load is just a convenience function. Suggesting you use the lower-level AJAX functions for more flexibility. But as a convenience function, if it works for you, that's fine.result = 'rgba(' + r + ',' + g + ',' + b + ',' + opacity / 100 + ')';String cocatenation in JS is really messy with all that quotes. Here's an alternative:
var rgba = [r,g,b,opacity/100].join(',');
result = 'rgba(' + rgba + ')';var delegateLoginModal = new modal("90%", "", "", "", true, "");
// create your events
$('body').on('click', '.showModal', function (e) {
e.preventDefault();
delegateLoginModal.showModal();
});Suggesting you look at how Bootstrap implemented their modal. It's in a form of a jQuery plugin. Your code looks cleaner if it did something as clean as:
$('someElement').modal(/*config|method*/);Code Snippets
function modal(width, height, backgroundColor, overlayColor, showOverlay, url) {new Modal({
width : 100,
height : 100
});function Modal(options){
options = $.extend(options,{
width : /*default width */,
height : /*default height */
});
...
}this.createModal = function () {...};
this.themeModel = function () {...};$('.modalOverlay').css({
'background-color': overlayBgColor
});
$('.modalContent').css({
'background-color': this.backgroundColor,
'width': this.width,
'height': this.height
});Context
StackExchange Code Review Q#70173, answer score: 5
Revisions (0)
No revisions yet.