Finish styling

[nicinabox]

A checklist for tracking/finishing styling.

• box-shadow still visible after hiding devtools
• in hidden state, DebugPanel is still visible (Ctrl+H doesn't hide DevTools container #54)
• add hover state on LogMonitorEntryAction title
• add vertical margin on deeply nested values
• change error color to red (Change error color #59)
• does action section need more callout?
• animate appending items (@threepointone?)
• key may be difficult to expand (I noticed the actions block has an odd conflict with the mouse)

[gaearon]

Currently action contents is highlighted because of a little bug. It's the same highlight that highlights the state changes. However I decided to release that as is, because it's actually better (to me) when the action is highlighted. After all it's the reason something changes. We can fix the bug, but then I think action section should have a lighter background so there's more accent on it.

What do you think?

[gaearon]

I was also wondering if you'd like to consider using react-motion for some animations. For example a very subtle "add" animation could help distinguish just inserted actions. I wonder if @threepointone is interested in experimenting with something like this.

[nicinabox]

My thoughts were that action was already priority by nature of being first. I changed the whitespace around the content sections so the next section (state) is farther away, giving action a tad more weight. With my short exploration last night the background seemed to add more noise than clarity.

Reminds me of that quote:

If you make everything bold, nothing is bold

Right now all the backgrounds have at least one reason for existing:

• Title backgrounds function as a visual divider and denote (more subtly) an action
• Button backgrounds are a visual divider and action
• Highlight backgrounds show a change in a sea of content that's difficult to cross reference without it

An action content background says "look at me!", but provides no significant improvement in usability, imo.

[gaearon]

Fair. Would you like me to fix the bug then?

[nicinabox]

+1 animations would be a nice touch. It's a little hard to tell you're at the bottom when a new item its appended. I keep scrolling the wrong direction.

[gaearon]

Idea: we can add “previous value” tooltip to changed primitive values on hover.

[nicinabox]

I like it.

[nicinabox]

Or what about like git style? Cross out the previous value, show in red. Show the new value next to it in green. Then you can do away with the extra background too.

[gaearon]

Or what about like git style? Cross out the previous value, show in red. Show the new value next to it in green. Then you can do away with the extra background too.

Interesting, we can try this. I'm busy with other stuff but I'm happy to see people contribute. (Tip: you already have access to previousValue in each node, that's what it uses to change highlighting.)

[nicinabox]

I'll do a mockup of both this evening to get a better sense.

On Wed, Aug 12, 2015 at 11:39 Dan Abramov notifications@github.com wrote:

Or what about like git style? Cross out the previous value, show in red.
Show the new value next to it in green. Then you can do away with the extra
background too.

Interesting, we can try this. I'm busy with other stuff but I'm happy to
see people contribute. (Tip: you already have access to previousValue in
each node, that's what it uses to change highlighting.)

—
Reply to this email directly or view it on GitHub
#52 (comment)
.

[dzannotti]

I wanted to push react-motion into this myself, totally +1

[threepointone]

you have all my attention :) started trawling through the source (so nicely written). I think TransitionSpring will work well with the actions log. for now, I made a quick PR with a css fadeIn, what do you think? #57

[bkniffler]

Hey, great work on the devtools, I noticed an issue, see comment.

JSONObjectNode.prototype.getChildNodes = function getChildNodes() {
    if (this.state.expanded && this.needsChildNodes) {
      var obj = this.props.data;
      var childNodes = [];
      for (var k in obj) {
        if (obj.hasOwnProperty(k)) {
          var prevData = undefined;
          // should be:
          // if (typeof this.props.previousData !== 'undefined' && this.props.previousData !== null)
          // or why not if (!this.props.previousData)?
          if (typeof this.props.previousData !== 'undefined') {
            prevData = this.props.previousData[k];
          }
          var node = _grabNode2['default'](k, obj[k], prevData, this.props.theme);
          if (node !== false) {
            childNodes.push(node);
          }
        }
      }
      this.needsChildNodes = false;
      this.renderedChildren = childNodes;
    }
    return this.renderedChildren;
  };

[dzannotti]

hi @bkniffler i think you are looking at the babel compiled code rather than the source, the line in question is https://github.com/gaearon/redux-devtools/blob/master/src/react/JSONTree/JSONObjectNode.js#L61 the reason it checks for undefined compared to if(! is to pass down any previous value as they were (null/false being a valid value for previous data)

[bkniffler]

Okay, though I get this error here if the previous value was null:

[dzannotti]

interesting, thanks for this. will look into it

[josebalius]

@dzannotti @bkniffler I have the same error so i thought i would point it out here. It seems like the condition should be

if(typeof this.props.previousData !== 'undefined' && this.props.previousData !== null)

Otherwise it will break if you default an object to null and fill it in later with properties

[nicinabox]

@josebalius @bkniffler You might want to create an new issue for that. This thread is really about styling and usability.

[gaearon]

Closing for inactivity, most of these have been done, and the rest is welcome as PRs.