2
\$\begingroup\$

To clarify: The code in "What I achieved" is working. I'm looking for suggestions so I can improve.

I'm quite new with JQuery (and javascript) and I've been learning while styling a forum. Recently I came across a section of information that is added automatically from a form (its the output). I can't manipulate the HTML directly and I wanted to change the style of that output to go with my design. So I decided to use JQuery (and css) to accomplish this.

The code that I wrote works just fine, however I know it's not the cleanest nor the more efficient way. I thought maybe you could give me some feedback on the JQuery code. For example, the part where I delete the 'Race' and 'Other activity' tags: would be set in just one loop using a for or each? or, since its a code that will be in all the pages of the site, does it matter if I wrap all the code in just one function or its ok just like that?

Some notes:

  • All the Values (Value_1, Value_2, …) may change, they are the answers of the users. They can be numbers or text.

  • The Output of the form is the code in the "Original" class. The HTML structure is already written and I cannot change that. The rest of the HTML (.info, p, .moreinfo) was added by me.

  • The "What I want to achieve" is the html/css style that I created to go with the design of the site.

So what I have initially is this:

<div class="info">

<div class="original"><span class="label"><span style="color:#D0C6C4;">Race</span> : </span>Value_1<span class="label"><span style="color:#D0C6C4;">Age</span> : </span>Value_2<span class="label"><span style="color:#536482;">Nationality</span> : </span>Value_3<span class="label"><span style="color:#536482;">Points</span> : </span>Value_4<span class="label"><span style="color:#536482;">Messages</span> : </span>Value_5<span class="label"><span style="color:#536482;">Other activity</span> : </span>Value_6</div>

<p></p>
</div>
<div class="moreinfo"></div>

What I want to achieve is this:

<div class="info">
  <div class="pair">
    <strong>Age</strong><span>Value_2</span>
    <strong>Nationality</strong><span>Value_3</span>
  </div>
  <div class="pair">
    <strong>Points</strong><span>Value_4</span>
    <strong>Messages</strong><span>Value_5</span>
  </div>
                    
  <p>Value_1</p>
</div>

<div class="moreinfo">Value_6</div>

What I achieved:

<div class="info">
  <div class="pair">
    <span class="span-0">Age</span><span class="value span-1">Value_2</span>
    <span class="span-2">Nationality</span><span class="value span-3">Value_3</span>
  </div>
  <div class="pair">
    <span class="span-0">Points</span><span class="value span-1">Value_4</span>
    <span class="span-2">Messages</span><span class="value span-3">Value_5</span>
  </div>
                    
  <p>Value_1</p>
</div>

<div class="moreinfo">Value_6</div>

My code:

$(document).ready(function() {
 
 //First I deleted de attributes and the ":"
 $('.original span').each(function() {
   $(this).removeAttr('style');
   $(this).removeClass();
   
   var label = $(this).text();
   label = label.replace(' : ','');
   $(this).html(label);
 });
 
 //Wrap the text that is alone (without tags) inside .original
 $('.original').contents().filter(function(){return this.nodeType !=1}).wrap('<span class="value"></span>');
  
 //Deleted 'Race' and 'Other activity' spans, and positioned the info related to them (Value_1 and Value_6) to the place I want
 $('.original span').each(function() {
  label = $(this).text();
  if(label == 'Race'){
     var race = $(this).next().text();
     $('.info p').append(race);
     $(this).next().remove();
     $(this).remove();
   };
  if(label == 'Other activity'){
    var job = $(this).next().text();
    $('.moreinfo').append(job);
    $(this).next().remove();
    $(this).remove();
  }
});

//Wrap in pairs
var set = $('.original').children();
for(var i=0, len = set.length; i < len; i+=4){
  set.slice(i, i+4).wrapAll('<div class="pair"></div>');};

//Add extra class for each <span> so I can later style the grid-area in CSS
$('.pair').each(function() {
  $(this).children().addClass(function(index){return 'span-' + index});
});

//"Deleted" the .original div
$('.info > .original').contents().unwrap();
});

The CSS:

.info {
    width: 250px;
    height: 300px;
    display: flex;
    flex-direction: column;
    align-items: center;
    justify-content: space-around;
}

.info .pair {
    display: grid;
    grid-template-areas: "a b" "c d";
    column-gap: 10px;
    row-gap: 5px;
    grid-template-columns: 1fr 1fr;
}

.pair span.span-0 {grid-area: a;}
.pair span.span-2 {grid-area: b;}
.pair span.span-1 {grid-area: c;}
.pair span.span-3 {grid-area: d;}

.info span.value {
    background-color: #111;
    font-size: 14px;
}

.pair span.span-0, .pair span.span-2 {
    color: salmon;
    font-size: 12px;
    font-weight: bold;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ for starters I would you should use output and label instead of using spans when you have dedicated semantic tags for it. \$\endgroup\$
    – tacoshy
    Sep 11, 2023 at 12:57
  • \$\begingroup\$ I'll keep that in mind, thanks! However in this case, if you are referring to the class original, I can't do much. The forum is giving me that automatically and I cannot change that part of the code. \$\endgroup\$
    – Lucas
    Sep 24, 2023 at 12:51

1 Answer 1

2
\$\begingroup\$

Some quick suggestions after reviewing your code:

  1. Chain jQuery methods: You can chain methods to reduce the number of times you traverse the DOM.

  2. Use .text() instead of .html(): If you're not dealing with HTML tags, it's safer to use .text().

  3. Use .filter(): You can use .filter() to remove specific elements based on their text content.

  4. Use .appendTo() and .remove(): Instead of .append() and .next().remove(), you can directly move elements.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks, this is really useful! About Chaining in JQuery, I just realice I had a misconception about it. So… if I understood right, is it correct then to write, for example: $(this).removeAttr('style').removeClass(); instead of $(this).removeAttr('style'); $(this).removeClass();, right? \$\endgroup\$
    – Lucas
    Sep 24, 2023 at 13:04

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.