3
\$\begingroup\$

I have a button (or icon) which can have three states.

On hover, it changes state and on click, the new state gets transferred to another state. This state is kept until you click again for which it gets back to normal.

The code is working, but it does not look or feel good. Is there any way I can re-factor this?

HTML

<div class="rendering-expert request-list-supported">
    <i class="fa fa-cog">1</i>
    <a class="hidden" rel="request-list-add" href="/add/1">
        <i class="icon fa fa-plus-circle">2</i>
    </a>
    <a class="hidden" rel="request-list-remove" href="/remove/1">
        <i class="icon fa fa-check-circle">3</i>
    </a>
</div>

CSS

.hidden { display: none; }

JQuery

$(function() { 
    $(".rendering-expert.request-list-supported").hover(function() {
        if (!$(this).hasClass("has-request")) {
            $(this).find(".fa-cog").hide();
            $(this).find("[rel='request-list-add']").removeClass("hidden");
        }
    }, function() {
        if (!$(this).hasClass("has-request")) {
            $(this).find(".fa-cog").show();
            $(this).find("[rel='request-list-add']").addClass("hidden");
        }
    }).on("click", "[rel='request-list-add']", function(e) {
        e.preventDefault();
        var toggler = $(this);
        var href = toggler.attr("href");
        toggler.parent().find(".fa-cog").hide();
        toggler.next("[rel='request-list-remove']").removeClass("hidden");
        toggler.toggleClass("hidden");
        toggler.parent().toggleClass("has-request");
    }).on("click", "[rel='request-list-remove']", function(e) {
        e.preventDefault();
        var toggler = $(this);
        var href = toggler.attr("href");
         toggler.parent().find(".fa-cog").show();
         toggler.prev("[rel='request-list-remove']").addClass("hidden");
         toggler.addClass("hidden");
         toggler.parent().removeClass("has-request");
    });
});

Please note that some parts are stripped in this example. It will only change state in my production if an AJAX request succeeds.

\$\endgroup\$
8
  • \$\begingroup\$ Is there a semantic rationale for writing the HTML that way (<div> with three children)? Would it also be acceptable to just have one element whose class changes for each state? \$\endgroup\$ Aug 15, 2014 at 6:12
  • \$\begingroup\$ The div has a lot of other properties. Just stripped a lot for this example \$\endgroup\$ Aug 15, 2014 at 6:20
  • \$\begingroup\$ May I suggest that you post the real thing, or something very close to it? There's no sense seeking advice that won't be useful to you. \$\endgroup\$ Aug 15, 2014 at 6:23
  • \$\begingroup\$ Hi again, see: jsfiddle.net/6qwt2vf8/1 \$\endgroup\$ Aug 15, 2014 at 6:32
  • \$\begingroup\$ There should be a functionaæ \$\endgroup\$ Aug 15, 2014 at 15:21

1 Answer 1

2
\$\begingroup\$

Since jquery event handler is a memory and processing "eater" because has to listen for the events then I think is better to delegate the hover responsibility to css. The change is just remove class="hidden" from your elements and then set the right selectors to user display:none to hide or display:inline to show - I use inline instead of block since it is applied to anchor tag and the default behavior for this tag is inline.

With this change then javascript can be simplified and the hover handlers can be removed.

BTW you still have to implement the click handler to add the has-request class.

The code should look likes the following,

In your css:

.rendering.rendering-expert.request-list-supported:hover [rel=request-list-add],
.rendering.rendering-expert.request-list-supported.has-request [rel=request-list-remove] 
{
    display: inline
}
.rendering.rendering-expert.request-list-supported [rel=request-list-add],
.rendering.rendering-expert.request-list-supported [rel=request-list-remove],
.rendering.rendering-expert.request-list-supported:hover .fa-cog,
.rendering.rendering-expert.request-list-supported.has-request .fa-cog,
.rendering.rendering-expert.request-list-supported.has-request [rel=request-list-add]
{
    display: none
}

And then replace your js:

$(function() { 
    $(".rendering-expert.request-list-supported").on("click", function(e) {
        e.preventDefault();
        var toggler = $(this);
        toggler.toggleClass("has-request");
    });
});
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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