2
\$\begingroup\$

I'm building a GroupPicker class that essentially manages an interface that allows the user to move items between two columns "assigned" vs "unassigned". The items within the columns have a hierarchy of parents and children, so if a child is assigned, so is the parent.

What I'm doing could be completely fine, but it seems like a code smell to me to have the HTML coupled into the class itself. The class needs to be flexible because sometimes the items are not tiered, sometimes we don't want to be able to remove already assigned items, etc., and having it coupled with the HTML seems to make it pretty rigid.

What is the best practice when dealing with a class that's sole purpose is to manage the state of the HTML and the values of the inputs?

Imgur

<!-- The HTML Structure -->

<div data-grouppicker="grouppage">
    <div>
        <div>Available Contact Groups</div>
        <div>
            <ul id="unassigned">
            <?php foreach ( $DropDownItems as $Item ) { ?>
                <?php [$ID, $Description, $TreeLevel, $ParentID, $HasChildren, $ChildrenIDs, $Count] = explode( ',', $Item ); ?>
                <?php $IsAssignedParent = ( ! in_array( $ID, $UnassignedIDArray ) && $HasChildren ); ?>
                <?php if ( in_array( $ID, $UnassignedIDArray ) ) { ?>
                    <li
                        id="list<?php echo $ID; ?>"
                        <?php echo $IsAssignedParent ? 'class="disabledPickListItem"' : ''; ?>
                        data-id="<?php echo $ID; ?>"
                        data-counter="<?php echo $Count; ?>"
                        data-parent="<?php echo $ParentID; ?>"
                        data-children="<?php echo $ChildrenIDs; ?>"
                        data-treelevel="<?php echo $TreeLevel; ?>"
                        data-disabled="<?php echo $IsAssignedParent ? 'true' : 'false'; ?>"
                    >
                        <div><?php echo $Description; ?></div>
                        <img src="images/add_icon.png" data-assign="<?php echo $ID; ?>" />
                    </li>
                <?php } ?>
            <?php } ?>
            </ul>
        </div>
    </div>
    <div>
        <div>Assigned Contact Groups</div>
        <div>
            <ul id="assigned">
            <?php foreach ( $DropDownItems as $Item ) { ?>
                <?php [$ID, $Description, $TreeLevel, $ParentID, $HasChildren, $ChildrenIDs, $Count] = explode( ',', $Item ); ?>
                <?php if ( in_array( $ID, $AssignedIDArray ) ) { ?>
                    <li
                        id="list<?php echo $ID; ?>"
                        data-id="<?php echo $ID; ?>"
                        data-counter="<?php echo $Count; ?>"
                        data-parent="<?php echo $ParentID; ?>"
                        data-children="<?php echo $ChildrenIDs; ?>"
                        data-treelevel="<?php echo $TreeLevel; ?>"
                        data-disabled="false"
                    >
                        <div><?php echo $Description; ?></div>
                        <img src="images/remove_icon.png" data-unassign="<?php echo $ID; ?>" />
                    </li>
                <?php } ?>
            <?php } ?>
            </ul>
        </div>
    </div>
    <input type="hidden" id="assignedValues" name="assignedValues" value="<?php echo implode(',', $AssignedIDArray); ?>" autocomplete="off" />
    <input type="hidden" id="unassignedValues" name="unassignedValues" value="<?php echo implode(',', $UnassignedIDArray); ?>" autocomplete="off" />
</div>

<script>var gp = new GroupPicker(document.querySelector('[data-grouppicker="grouppage"]'));</script>

 

Here is the start of the GroupPicker class. The constructor, _init() and assign:

// GroupPicker.js

function GroupPicker( element ) {
   var self = this;

   self.el = element;
   self.elUnassignedInput = self.el.querySelector( 'input#unassignedValues' );
   self.elAssignedInput = self.el.querySelector( 'input#assignedValues' );

   self._init();
}

GroupPicker.prototype._init = function() {
   var self = this;

   self.el.addEventListener( 'click', function( event ) {
      if ( event.target.dataset.assign ) {
         var elItem = self.el.querySelector( 'ul#unassigned li[data-id="' + event.target.dataset.assign + '"]' );
         self.assign( elItem );
      } else if ( event.target.dataset.unassign ) {
         var elItem = self.el.querySelector( 'ul#assigned li[data-id="' + event.target.dataset.unassign + '"]' );
         self.unassign( elItem );
      }
   });

   var disabledUnassigned = [].slice.call( self.el.querySelectorAll( 'ul#unassigned li[data-disabled="true"]' ) );
   disabledUnassigned.forEach( function( elItem ) {
      self._checkDescendants( elItem );
   });
};

GroupPicker.prototype.assign = function( elItem ) {
   var self = this;
   var itemId = elItem.dataset.id;

   self.unassigned = [].slice.call( self.el.querySelectorAll( 'ul#unassigned li' ) );
   self.assigned = [].slice.call( self.el.querySelectorAll( 'ul#assigned li' ) );

   self._addToAssignedInput( itemId );
   self._removeFromUnassignedInput( itemId );

   self._cloneToAssignedList( elItem );
   self._checkDescendants( elItem );
   self._handleParent( elItem );
};

I think the best example of my problem is in the method _deleteParent:

GroupPicker.prototype._deleteParent = function( elParent ) {
   var self = this;
   self.unassigned = [].slice.call( self.el.querySelectorAll( 'ul#unassigned li' ) );
   var elParentDescendants = elParent.dataset.children.split( '|' );
   var hasUnassignedDescendants = self.unassigned.some( function( elUnassignedItem ) {
      return elParentDescendants.indexOf( elUnassignedItem.dataset.id ) !== -1 && elUnassignedItem.dataset.disabled == 'false';
   });
   var elItemHasParent = Number( elParent.dataset.parent );
   var elItemParent = self.el.querySelector( 'ul#unassigned li[data-id="' + elParent.dataset.parent + '"]' );

   if ( ! hasUnassignedDescendants ) {
      elParent.parentNode.removeChild( elParent );
   }

   if ( elItemHasParent && elItemParent ) {
      self._deleteParent( self.el.querySelector( 'ul#unassigned li[data-id="' + elParent.dataset.parent + '"]' ) );
   }
};

When the last descendant of an item is assigned, the disabled ancestors are removed. You can see examples of assumptions made about the HTML structure that make me inherently nervous like: var elItemParent = self.el.querySelector( 'ul#unassigned li[data-id="' + elParent.dataset.parent + '"]' );.

If you had to write a class that had similar responsibilities, how would you go about writing it? Thanks for anything you can teach me--I'd love good resources about this as well. I wasn't able to find anything from googling about it.

EDIT: Even though I'm providing an example here, I think the root of my question doesn't require a code review per se. It boils down to a question of how we write classes that manage the state of a DOM and its data and if the methods on the classes modifying the state directly is a smart thing to do, especially when making assumptions about its structure when it doesn't have control over that aspect.

\$\endgroup\$
2
\$\begingroup\$

You're right that you shouldn't make assumptions about the DOM when writing your state manager. Actually you should start by writing just your state manager, without worrying how that state should be reflected in the DOM.

When your state manager works, you can write a separate class that does the DOM manipulation - based on the state in your state manager class. This is actually called an MVC pattern. A Model class represents the State. A View class represents the DOM view, and a Controller class decides how the state should be represented in the View.

If you google for "managing state" and "MVC" you'll find a lot of resources, and libraries that already fix this problem for you :)

For starters, I suggest not mixing javascript and php too much, this will get confusing really quickly. Also, generally speaking, do not use CSS classes to find out in what state your app is. CSS is only used to represent state, not to keep track of it.

\$\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.