1

I am trying to unselect the other div i click on before. i have stored the old div id in an hidden input as you can see below i have added a alert so show the last div selected and it works fine but as soon as i try and change the div text color in css the whole script stops working.

 function edit_addon(div_id) {   
 $("#"+div_id).attr('contentEditable', true);
 $("#"+div_id).html()
 $("#"+div_id).css('color','#F00');
 $("#"+div_id).css('cursor','Text');


 var old_div = $("input#selected_div").val();
 alert(old_div);
 $("#"+old_div).css('color','#000');

 $("input#selected_div").val(div_id);

 } 
Rickstar
  • 6,057
  • 21
  • 55
  • 74

3 Answers3

1

Are you sure the alert works fine? It should not work on the first pass because there was no value for selected_div until the first call to edit_addon.

Modify the code to:

function edit_addon (div_id) {   
    $("#"+div_id).attr ('contentEditable', true)    //-- Use no semicolon here
                 .css ('color','#F00')
                 .css ('cursor','Text')
                 ;

    var old_div = $("input#selected_div").val();
    alert(old_div);
    if (old_div)
        $("#"+old_div).css('color','#000');

    $("#selected_div").val(div_id);
} 

.
You can see it in action at jsfiddle. If you do, note that by changing the function to an event handler (as shown at jsfiddle) you can simplify the HTML and bind the function in a progressive-enhancement way.

Community
  • 1
  • 1
Brock Adams
  • 90,639
  • 22
  • 233
  • 295
0

It seems to work fine.

function edit_addon(div_id) {
                $("#"+div_id).attr('contentEditable', true)
                .css('color','#F00');
                .css('cursor','Text');


                var old_div = $("input#selected_div").val();
                alert(old_div);
                $("#"+div_id).css('color','#000');

                $("input#selected_div").val(div_id);

            }

<div id="hello" onclick="edit_addon('hello')">HelloWorld!</div>
<div id="bye" onclick="edit_addon('bye')">Bye!!</div>
<form><input type="hidden" id="selected_div" /></form>

I used this and there doesn't seem to be a problem.

JC.
  • 629
  • 3
  • 8
  • 13
  • you know where you got $("#"+div_id).css('color','#000'); can you change it to $("#"+old_div).css('color','#000'); and try it for me it dose not work. – Rickstar Sep 05 '10 at 20:40
0

I have a couple of pointers to make this a little easier to write and to improve your existing code.

First, lines 1 - 4 keep selecting your desired element over any over again - this means more DOM traversals, which is slower. All of the jQuery methods return the instance of the element(s), so you can "chain" your method calls like this:

$("#"+div_id).attr('contentEditable', true)
    .css('color','#F00')
    .css('cursor','Text');

Note: The line breaks here are insignificant.

Second, selectors of the form #myId generally perform better than selectors of form tagnam#myId, the reason being the first will map directly to document.getElementById whereas the second will use document.getElementsByTagName and then loop over them to find the one with the id.

So, back to your original question. My suggestion is to simply add a CSS class to your selected element, rather than setting inline styles with the call to .css(). Then, there is no need to store your previous element in a hidden field like you've suggested. Instead, you can simply just select the div tags that have your selected class. The key here is that your previous element, if any, will always have the CSS class you're adding.

Here's an example of what I'm getting at:

function edit_addon(div_id)
{
    //step 1 - find your old element and clear the CSS
    $('div.selected').removeClass('selected');
    //step 2 - now, modify the div_id element
    $("#"+div_id).attr('contentEditable', true)
        .addClass('selected');
} 
wsanville
  • 37,158
  • 8
  • 76
  • 101