Minor Refactoring under Test in Dart

‹prev | My Chain | next›

Today, I would like to start thinking about how to consolidate some of my Dart code in the ICE Code Editor. I think that it is a bit early to start breaking the sub-menus and dialogs out into Dart classes. So instead, I am going to scavenge the codebase for things that are asking to be removed or consolidated.

One of the first things that asks to come out are styles. I love using method cascades for CSS styles in Dart:

  _openNewProjectDialog() {
    var dialog = new Element.html(
        '''
        <div class=ice-dialog>
        <label>Name:<input type="text" size="30"></label>
        <button>Save</button>
        </div>
        '''
      );

    dialog.style
      ..position = 'absolute'
      ..margin = '2px'
      ..right = '20px'
      ..top = '45px'
      ..zIndex = '999';
    // ...
  }

As much as I like a good method cascade, I am setting the same style for multiple dialogs. In fact, I am setting the same style for sub-menus as well. So, reluctantly, I pull that out into vanilla CSS:

.ice-menu, .ice-dialog {
  /* ... */
  position: absolute;
  margin: 2px;
  right: 20px;
  top: 45px;
  z-index: 999;
}

Sometimes there is no substitution for a low-tech solution.

I clean out a few other minor things and then come across:

  _hideMenu() {
    if (query('.ice-menu') == null) return;
    query('.ice-menu').remove();
    // queryAll('.ice-menu').forEach((e)=> e.remove());
  }

I really dislike that conditional. It seems like that queryAll() ought to be equivalent, if not a better solution. Instead of finding one .ice-menu element, it will find them all and remove each. If there are no matching elements, which is what the current conditional is guarding, then the forEach() should be a no-op.

But I commented that out for a reason. Specifically, if I replace the current code with the queryAll() version, I get test failures:

FAIL: project menu lists project names
  Expected: List of elements contains match 'My New Project'
       but: Element list content was <[☰X, ☰, X, , , , , , , , , , , , , X, , , ]>.

Interestingly, I think that I was relying on coincidence to make the “list project names” feature work. In particular, the way that I opened the project list sub-menu was opening the menu first, then closing the main menu:

  _projectsMenuItem() {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _openProjectsMenu())
      ..onClick.listen((e)=> _hideMenu());
  }

This, and a bunch of other tests, pass if I hide all menus before opening the project menu:

  _projectsMenuItem() {
    return new Element.html('<li>Projects</li>')
      ..onClick.listen((e)=> _hideMenu())
      ..onClick.listen((e)=> _openProjectsMenu());
  }

In the end, the previous conditional-then-remove-the-first-element-in-the-DOM approach was less than robust. So this refactoring / cleanup has helped improve the strength of the codebase. Hopefully it has cleaned enough cruft so that I have a better handle on how to better approach those sub-menus and dialogs. I will get started on that tomorrow.


Day #760