-2
\$\begingroup\$

Everything works, but my hard coding is worse:

Codepen

<div class="header__search">
<form action="">
  <div class="search__input-wrapper">
    <div class="search__dropdown">
      <div class="search__dropdown-trigger btn-default">
        <span>Portfolio</span>
        <i class="search__dropdown-icon"></i>
      </div>
      <ul class="search__dropdown-list">
        <li class="dropdown-list__item">portfolio</li>
        <li class="dropdown-list__item">blog</li>
      </ul>
    </div>
    <input type="text" class="search__input" placeholder="Search in portfolio">
    <i class="search__icon search__icon-blog"></i>
    <i class="search__icon search__icon-portfolio"></i>
  </div>
  <input type="submit" class="search__submit btn-submit" value="Go">
</form>

$(document).ready(function () {


    // Class with methods to choose search category
    var chooseCategory = {

        getDropdownMenu: function(){
                $('.search__dropdown-trigger').click(function(e) {
                e.preventDefault();
                var categoryList = $(this).next('.search__dropdown-list');

                if (categoryList.prop('hidden')) {;
                    categoryList.removeProp('hidden');
                }else{
                    categoryList.prop('hidden','hidden');
                }
            });
        },

        chooseDropdownItem: function() {
            $('.dropdown-list__item').click(function () {
                var name = $(this).text();
                var categoryList = $(this).parent('.search__dropdown-list');

                // For this need better separate method, but...
                // Impure function((((
                $('.search__dropdown-trigger span').text(name);
                $('.search__input').attr("placeholder", "Search in "+name);
                if(name=='portfolio'){
                    $('.search__icon-portfolio').show();
                    $('.search__icon-blog').hide();
                }else{
                    $('.search__icon-blog').show();
                    $('.search__icon-portfolio').hide();
                }
                // $('.search__icon').toogle('search__icon-'+name);
                categoryList.prop('hidden','hidden');
            });
        },

        // replaceText: function () {
        //  $('.search__dropdown-trigger').text(this.chooseDropdownItem());
        // },

        init: function(){
            this.getDropdownMenu();
            this.chooseDropdownItem();
        }



    };

    chooseCategory.init();

});

This code must do:

After click on .search__dropdown-trigger appear .search__dropdown-list. In this list we have two categories "Blog" and "Portfolio", after click on some of them in .search__dropdown-trigger>span paste the relevant text node, show the relevant icon .search__icon-blog or .search__icon-portfolio and change input's placeholder "Search in portfolio".

\$\endgroup\$
5
\$\begingroup\$

Create a Private Scope

The first thing I would do it to move your code into a closure by using an IIFE. This will help prevent collisions between your code and any other code by creating a private scope. It also prevents your code from polluting the global scope. One other thing it will allow you to do is to refer to jQuery as $.

You should also use strict mode for your code. By using this, it forces the JavaScript engine to prevent common mistakes in your code (like not varing your variables and a lot more things).

(function( $, undefined ) {
  //undefined is needed only for really old browsers
  'use strict';
  // your code goes here
})( jQuery );

Cache Your Selectors

The next thing I would recommend is caching your jQuery selectors. This will improve the overall performance of your code. I tend to preface my cached selectors with a $ so I know they are jQuery objects.

var $search = $('header__search');
var $trigger = $search.find('.search__dropdown-trigger');
var $categoryList = $search.find('.search__dropdown-list');

By caching the header__search object, jQuery only has to search this container instead of the entire DOM every time. A basic rule of thumb is, if you use a selector more than once, cache it.

Updating getDropDownMenu

Next, your getDropDownMenu function can be simplified if you just toggle the hidden property instead of removing it.

  var isHidden = !( $categoryList.prop('hidden') ); //swap value btwn true/false
  $categoryList.prop('hidden',  isHidden );

Updating chooseDropDownItem

You can change your chooseDropDownItem function similarly by using the toggle method (jQuery API) for the icons. The toggle method can take a boolean value to determine whether to display or not. For example:

$('.search__icon-portfolio').toggle( ( name === 'portfolio' ) );
$('.search__icon-blog').toggle( (name === 'blog') );

So in the above code, if name is "blog", then the boolean value is true for the search__icon-blogso the element will be displayed, but it would be false for the search__icon-portfolio. This can be simplified even further by chaining all of your selections together by using the cached $search variable above.

$search
  .find('.search__input').attr( 'placeholder', 'Search in ' + name ).end()
  .find('.search__icon-portfolio').toggle( ( name === 'portfolio' ) ).end()
  .find('.search__icon-blog').toggle( (name === 'blog') );

Then end method (jQuery API) returns you back to your previous selection (in this case $search). Also, notice I used the identity operator (===) instead of the equality operator (==). I recommend doing this in all cases.

Use .on For Events

You should also use the more current on method (jQuery API) instead of the old click method. The click method calls on behind the scenes anyway so just skip the middleman. Also, I would recommend using delegated events instead of creating click events on each dropdown item. This will improve performance as well since there are fewer events being tracked on the page.

$categoryList.on( 'click', '.dropdown-list__item', function( e ) {
 // code here
}

This function uses the cached selector from above. When a dropdown-list__item is clicked, it bubbles up to the parent element and we handle the event there. This way there is only one click event for ALL of the items.

Simplify document.ready

Also, I would remove most of your code from the document.ready event. It is unnecessary. Go ahead and declare your functions before the event and only call the init method inside of it.

$(function() {  //shorthand for $(document).ready
  chooseCategory.init();
});

Public and Private Methods

Last, I would assume that your chooseDropDownItemand getDropdownMenu are private methods and only your init method is a public method. You can make the first two private methods by placing them outside of your chooseCategory object. Since they are all part of the same scope, the init function will still have access to them but they will be unavailable globally.

function chooseDropdownItem() {}
function getDropdownMenu(){}
function init() {}
var chooseCategory = {
  init : init
};

Final Code

So, if you were to implement all of these changes, then your code would look like this:

(function( $, undefined ) {

  'use strict';

  var $search = $('header__search');
  var $trigger = $search.find('.search__dropdown-trigger');
  var $categoryList = $search.find('.search__dropdown-list');

  function init() {
    getDropdownMenu();
    chooseDropdownItem();
  }

  function getDropdownMenu(){
    $trigger.on( 'click', function( e ) {
      e.preventDefault();
      var isHidden = !( $categoryList.prop('hidden') );
      $categoryList.prop('hidden',  isHidden );
    });
  }

  function chooseDropdownItem() {
    $categoryList.on( 'click', '.dropdown-list__item', function( e ) {
    var name = $(this).text();
    $trigger.find('span').text( name );

    //adding this because I am not sure of your text values
    //remove if you don't need it
    name = name.toLowerCase();

    $search
      .find('.search__input').attr( 'placeholder', 'Search in ' + name ).end()
      .find('.search__icon-portfolio').toggle( ( name === 'portfolio' ) ).end()
      .find('.search__icon-blog').toggle( (name === 'blog') );
    $categoryList.prop( 'hidden', true );
    });
  }

  var chooseCategory = {
    init : init
  };

  $(function() {
    chooseCategory.init();
  });

})( jQuery );

I hope that helps. Let me know if you have any further questions.

\$\endgroup\$

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.