1
\$\begingroup\$

I made this code to change the background color for a div in a set of divs while disabling the rest of the divs in the background. It works fine, but I wanted the code reviewed for more improvements since I am new to jQuery and JavaScript.

<div id="singleProjectTitle">
    <div  class="ProjectMainLink" id="aboutProject"><div  class="ProjectMainLinkInnerDiv">عن المشروع</div></div>
    <div  class="ProjectMainLink" id="key"><div  class="ProjectMainLinkInnerDiv">المميزات الرئيسية</div></div>
    <div  class="ProjectMainLink" id="map"><div  class="ProjectMainLinkInnerDiv">الخارطة</div></div>
    <div  class="ProjectMainLink" id="galary"><div  class="ProjectMainLinkInnerDiv">البوم الصور والفيديو  </div></div>
</div>


  var count = $(".ProjectMainLink").length;
     for(var i=0; i<count;i++){
         $(".ProjectMainLink").eq(i).click(function() {
              $(".ProjectMainLink").css("background-color","");
            $(this).css("background-color","#fff");

        });
         }
\$\endgroup\$
2
\$\begingroup\$

For maintainability, avoid setting the background color directly in JavaScript / jQuery. Rather, you should keep all styling information in CSS. Your jQuery code should limit itself to adding or removing classes. Building on the answer by CodeX…

<html dir="rtl">
  <head>
    <style type="text/css">
      .ProjectMainLink.Selected {
          background-color: #000;
      }
    </style>
  </head>
  <body>
    <div id="singleProjectTitle">
      <div  class="ProjectMainLink" id="aboutProject"><div  class="ProjectMainLinkInnerDiv">عن المشروع</div></div>
      <div  class="ProjectMainLink" id="key"><div  class="ProjectMainLinkInnerDiv">المميزات الرئيسية</div></div>
      <div  class="ProjectMainLink" id="map"><div  class="ProjectMainLinkInnerDiv">الخارطة</div></div>
      <div  class="ProjectMainLink" id="galary"><div  class="ProjectMainLinkInnerDiv">البوم الصور والفيديو  </div></div>
    </div>
    <script type="text/javascript">
      $(document).on("click", ".ProjectMainLink", function() {
          $(".ProjectMainLink").removeClass("Selected");
          $(this).addClass("Selected");
      });
    </script>
  </body>
</html>

The CSS should be moved into a separate file rather than inlined in the proof-of-concept above.

The HTML could use some simplification as well. You'll have to decide whether this simplification is appropriate for you.

<html dir="rtl">
  <head>
    <style type="text/css">
      #singleProjectTitle > div.Selected {
          background-color: #000;
      }
    </style>
  </head>
  <body>
    <div id="singleProjectTitle">
      <div id="aboutProject">عن المشروع</div>
      <div id="key">المميزات الرئيسية</div>
      <div id="map">الخارطة</div>
      <div id="galary">البوم الصور والفيديو</div>
    </div>
    <script type="text/javascript">
      $(document).on("click", "#singleProjectTitle > div", function() {
          $("#singleProjectTitle > div").removeClass("Selected");
          $(this).addClass("Selected");
      });
    </script>
  </body>
</html>

galary should be spelled gallery.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Completely forgot about those Class functions, makes for cleaner code, V.nice \$\endgroup\$ – CodeX Jul 15 '14 at 17:23
  • \$\begingroup\$ 200_success♦ thanks for the review and the valuable hints it gave me more control over my script :)i cant remove the inner dives cuz they have their own style before selecting \$\endgroup\$ – yasir ALQAISI Jul 16 '14 at 10:12
2
\$\begingroup\$

im not sure why you would want the length or count of the class..
You could simply use this to highlight the divs:

    $(".ProjectMainLink").click(function() {
          $(".ProjectMainLink").css("background-color","");
          $(this).css("background-color","#000");

    });

JS FIDDLE: http://jsfiddle.net/p3m2p/

EDIT:

Don't forget to wrap your functions in <script type="text/javascript"></script>

It could also be done like this:

<script type="text/javascript">
$(document).on("click",".ProjectMainLink",function() {
          $(".ProjectMainLink").css("background-color","");
          $(this).css("background-color","#000");
    });
</script>
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks very much iam new to jquery :) thanks for the review it help me understand jquery more . \$\endgroup\$ – yasir ALQAISI Jul 15 '14 at 12:07
  • \$\begingroup\$ Added a full example with script tags included \$\endgroup\$ – CodeX Jul 15 '14 at 14:49

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.