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\$
  • 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\$ – Florian Margaine Jun 8 '12 at 8:38
  • \$\begingroup\$ @FlorianMargaine can you just point me what was not ok. \$\endgroup\$ – Priyank Patel Jun 8 '12 at 9:00
  • \$\begingroup\$ Just look at the edit: codereview.stackexchange.com/posts/12378/revisions \$\endgroup\$ – Florian Margaine Jun 8 '12 at 9:05
  • \$\begingroup\$ @FlorianMargaineya I checked that , I agreee it needs to be proper next time , I will take care of it. \$\endgroup\$ – Priyank Patel Jun 8 '12 at 9:07
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\$
  • \$\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\$ – Priyank Patel Jun 9 '12 at 5:14
  • \$\begingroup\$ Another question ,I need to call ShowSubmitPopUp() function and pass appropriate class name as arguments right? \$\endgroup\$ – Priyank Patel Jun 9 '12 at 5:22
  • \$\begingroup\$ Thanks a lot , it worked and the code is clean and less as well.Thanks. \$\endgroup\$ – Priyank Patel Jun 9 '12 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\$
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\$
  • \$\begingroup\$ thanks a lot for correcting my mistake , really helpful.Thanks. \$\endgroup\$ – Priyank Patel Jun 9 '12 at 5:17
  • 1
    \$\begingroup\$ You made no mistake - you're just learning and that is very good. \$\endgroup\$ – Robert Niestroj Jun 9 '12 at 6:25
  • \$\begingroup\$ You are right,getting feedbacks from better people is a blessing.Thanks mate. \$\endgroup\$ – Priyank Patel Jun 9 '12 at 6:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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