0
\$\begingroup\$

I am having a popup which is styled to match the page and am using that single popup to display messages to the user. My popup contains one h1 element and an ASP.NET Label.

The h1 element and Label contents and color change according to the operation: if the information was successfully inserted the h1 text becomes Success and Label's text becomes "Your information was saved".

On incorrect input it changes accordingly, and on some unexpected error it changes the same way.

Currently I am calling three different JavaScript functions on appropriate events from my code behind using RegisterClientScriptBlock.

HTML:

<div id="NewCustomerSubmitStatusPopUp">
 <a href="#" class="NewCustomerSubmitPopUpClose"><img class="NewCustomerSubmitPopUpImg" src="Images/cancel1.png"/></a>
 <h1></h1>
 <asp:Label ID="NewCustomerlblSubmitStatusMsg" runat="server" Text=""></asp:Label><br /><br />
 <input class="NewCustomercmdSubmitPopUp"  type="button" value="Close" />  
 </div>

CSS:

#NewCustomerSubmitStatusPopUp
    {
    display:none;
    position: fixed;
    width:500px;
    height: 150px;
    top: 50%;
    left: 50%;
    margin-left:-255px;
    margin-top:-110px;
    /*background-color:#F7DFDE; */
    padding:10px;
    z-index:102;
    font-family:Verdana;
    font-size:10pt;
    border-radius:10px;
    -webkit-border-radius:20px;
    -moz-border-radius:20px;
    font-weight:bold;
    }
    #NewCustomerSubmitStatusPopUp h1
    {
        /*color:#BD494A;*/
        padding:20px 0; 
        text-align:center; 
    }


    .NewCustomercmdSubmitPopUp
    {
        height:30px;
        width:50px;
        margin-left:220px;
    }

    .NewCustomerSubmitPopUpImg
    {
        float:right;
        margin-top:-30px;
        margin-right:-25px;
    }

javascript:

function ShowSubmitSuccessPopUp()
 {

    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: 'white' });
    $('#NewCustomerSubmitStatusPopUp').css({ border: '10px solid #9cc3f7'});
    $('#NewCustomerSubmitStatusPopUp h1').html("Success");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#9cc3f7' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#9cc3f7' });
}


function ShowSubmitErrorPopUp()
{

    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: '#F7DFDE' });
    $('#NewCustomerSubmitStatusPopUp').css({ border:'10px solid #BD494A'});
    $('#NewCustomerSubmitStatusPopUp h1').html("Error");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#BD494A' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').html("There are some errors in the page.Please rectify the errors by visiting the Errors tab.");
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#BD494A' });
}



function ShowSubmitUnexpectedErrorPopUp()
{
    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");
    $('#NewCustomerSubmitStatusPopUp').css({ background: '#F7DFDE' });
    $('#NewCustomerSubmitStatusPopUp').css({ border: '10px solid #BD494A' });
    $('#NewCustomerSubmitStatusPopUp h1').html("Something Went Wrong");
    $('#NewCustomerSubmitStatusPopUp h1').css({ color: '#BD494A' });
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').html("There was some error while submitting your information.Please re-enter the values and try again.");
    $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg').css({ color: '#BD494A' });
}

As you can see, for every event, I change text text of the h1 and the label element and change the style too.

Can anybody tell me if I am doing it in a good manner or if there is something wrong?

\$\endgroup\$
4
  • 1
    \$\begingroup\$ Next time, care about your grammar/spelling. It's ok not to speak english perfectly, but at least give some air to your text. \$\endgroup\$ Jun 8, 2012 at 8:38
  • \$\begingroup\$ @FlorianMargaine can you just point me what was not ok. \$\endgroup\$ Jun 8, 2012 at 9:00
  • \$\begingroup\$ Just look at the edit: codereview.stackexchange.com/posts/12378/revisions \$\endgroup\$ Jun 8, 2012 at 9:05
  • \$\begingroup\$ @FlorianMargaineya I checked that , I agreee it needs to be proper next time , I will take care of it. \$\endgroup\$ Jun 8, 2012 at 9:07

3 Answers 3

1
\$\begingroup\$

It's pretty much commented in the code:

JavaScript:

$(function() {
        //cache repeatedly used elements to avoid re-querying them           
    var customermask = $('#NewCustomerMask'),
        popup = $('#NewCustomerSubmitStatusPopUp'),
        //use context to get descendants of a target context. it's similar to a .find()
        popupHeader = $('h1', popup),
        placeholder = $('#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg'),
        //move out your text as variables. don't mingle them with the code
        errorMsg = 'There are some errors in the page.Please rectify the errors by visiting the Errors tab.',
        unexpectedErrorMsg = 'There was some error while submitting your information.Please re-enter the values and try again.';

    //condense the function to one
    function ShowSubmitPopUp(status) {

        //and do common tasks
        customermask.show('slow');
        //use classes to determine the state and style of the pop-up   
        popup.show('slow').addClass(status);
        placeholder.addClass(status);

        //use the status to determine specific tasks
        switch (status) {
        case 'success':
            //use .text() when setting only text
            popupHeader.text('Success');
            break;
        case 'error':
            placeholder.text(errorMsg);
            popupHeader.text('Error');
            break;
        case 'uerror':
            placeholder.text(unexpectedErrorMsg);
            popupHeader.text('Something went awfully wrong!');
            break;
        }
    }
});​

CSS:

/*since styles are common, you can stack them to avoid repeating*/

/*success styles*/
#NewCustomerSubmitStatusPopUp.success{
    background: #FFF;
    border: 10px solid #9cc3f7;
}

#NewCustomerSubmitStatusPopUp.success h1,
#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg.success{
    color: #9cc3f7;
}

/*error and uerror styles*/
#NewCustomerSubmitStatusPopUp.error,
#NewCustomerSubmitStatusPopUp.uerror{
    background: #F7DFDE;
    border: 10px solid #BD494A;
}

#NewCustomerSubmitStatusPopUp.error h1,
#NewCustomerSubmitStatusPopUp.uerror h1,
#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg.error,
#ctl00_ContentPlaceHolder1_NewCustomerlblSubmitStatusMsg.uerror{
     color: #BD494A;
}​​
\$\endgroup\$
3
  • \$\begingroup\$ I will give it a try and then come back to you , if I have any doubts.Thanks for the help.Really appreciate your effort. \$\endgroup\$ Jun 9, 2012 at 5:14
  • \$\begingroup\$ Another question ,I need to call ShowSubmitPopUp() function and pass appropriate class name as arguments right? \$\endgroup\$ Jun 9, 2012 at 5:22
  • \$\begingroup\$ Thanks a lot , it worked and the code is clean and less as well.Thanks. \$\endgroup\$ Jun 9, 2012 at 5:40
1
\$\begingroup\$

A few ideas:

  • Your three javascript functions could easily be combined into one with some simple arguments, like headerText, statusMessage, statusType.
  • In your description you list some success text, but this isn't in the javascript, so I guess this is set in the code-behind. I consolidate how the text is set. If you choose to use javascript, you could simplify things by changing the asp:Label to a span.
  • It might be nicer to put the style information in the CSS file and then in the javascript just toggle which classes apply.

Here's an example of how it might look:

CSS

#NewCustomerSubmitStatusPopup.successPopup
{
  background-color: white;
  border-color: #9cc3f7; /* width, pattern added to main CSS block */
}

#NewCustomerSubmitStatusPopup.successPopup h1, #NewCustomerSubmitStatusPopup.successPopup span
{
  color: #9cc3f7;
}

#NewCustomerSubmitStatusPopup.errorPopup
{
  background-color: #F7DFDE;
  border-color: #BD494A;
}

#NewCustomerSubmitStatusPopup.errorPopup h1, #NewCustomerSubmitStatusPopup.errorPopup span
{
  color: #BD494A;
}

javascript

function ShowSubmitPopUp(headingText, statusMessage, statusType)
{
    $('#NewCustomerMask').show("slow");
    $('#NewCustomerSubmitStatusPopUp').show("slow");

    var popupClass = (statusType == 'error' ? 'errorPopup' : 'successPopup');
    $('#NewCustomerSubmitStatusPopUp').attr('class', popupClass);
    $('#NewCustomerSubmitStatusPopUp h1').html(headingText);
    $('#NewCustomerSubmitStatusPopUp span').html(statusMessage);
}
\$\endgroup\$
0
1
\$\begingroup\$

Additionally to @CodeMonkey1 answer you can cache your jQuery objects for ex. $('#NewCustomerSubmitStatusPopUp') to a variable like this: var myObj = $('#NewCustomerSubmitStatusPopUp');

Then you can rewrite this code:

$('#NewCustomerSubmitStatusPopUp').show("slow");
$('#NewCustomerSubmitStatusPopUp').css({ background: '#F7DFDE' });
$('#NewCustomerSubmitStatusPopUp').css({ border: '10px solid #BD494A' });
$('#NewCustomerSubmitStatusPopUp h1').html("Something Went Wrong");
$('#NewCustomerSubmitStatusPopUp h1').css({ color: '#BD494A' });

to this:

myObj.show("slow");
myObj.css({ background: '#F7DFDE' });
myObj.css({ border: '10px solid #BD494A' });
myObj.find("h1").html("Something Went Wrong");
myObj.find("h1").css({ color: '#BD494A' });

This has the benefit that you dont have to query the DOM five times for the same thing.

\$\endgroup\$
3
  • \$\begingroup\$ thanks a lot for correcting my mistake , really helpful.Thanks. \$\endgroup\$ Jun 9, 2012 at 5:17
  • 1
    \$\begingroup\$ You made no mistake - you're just learning and that is very good. \$\endgroup\$ Jun 9, 2012 at 6:25
  • \$\begingroup\$ You are right,getting feedbacks from better people is a blessing.Thanks mate. \$\endgroup\$ Jun 9, 2012 at 6:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.