Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve UX of finding full key value #18702

Closed
karlhorky opened this issue Apr 22, 2020 · 23 comments · Fixed by #18737
Closed

Improve UX of finding full key value #18702

karlhorky opened this issue Apr 22, 2020 · 23 comments · Fixed by #18737

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Apr 22, 2020

The current behavior

The full value of the key is very difficult / impossible to find and use in the interface of the React Devtools.

Kapture 2020-04-22 at 11 47 47

Only managed to find it by accident :(

The expected behavior

The key is visible in the props list to the right.

Detailed Proposal

As mentioned below in #18702 (comment)

Add a light divider and new section in the props panel to the right.

Potentially also add a question mark that shows an explanation about the fact that things in this section are not really props.

Ref (Original implementation): facebook/react-devtools#328

@karlhorky karlhorky added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 22, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

This isn't a bug 😄 It's the intentional design.

The key is visible right in the Components tree. 😄 You're hovering over it a lot in the attached video.

The key isn't listed as a "prop" on the right because- at least for the moment, key isn't a prop. It won't be passed through to a component as part of the props object parameter.

And it's often useful to show inline because it's the only way to uniquely identify components in a list (like your DeleteLi) which would otherwise look identical and require you to select each one.

Sorry this caused confusion! If you have concrete suggestions for ways to improve the design, we're always happy to hear feeedback! Adding it to the Props panel would cause more confusion than it would help though, I think, since it's not a regular "prop".

@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

PS If you want to know the full value of a truncated attribute, you can double click to select it and then ctrl/cmd + C

@karlhorky
Copy link
Contributor Author

The key is visible right in the Components tree. 😄 You're hovering over it a lot in the attached video.

This issue is about the full value, which is super hidden (either by hovering for a long time or copying).

Would it be possible to get a second opinion on this questionable design decision?

@karlhorky
Copy link
Contributor Author

Having an inline button that appears on hover to show the full value would also be acceptable, in case it's not possible to put it somewhere in the right pane.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

Hovering for "a long time" means just hovering for the same amount of time the OS+browser takes to show tooltips, right? (DevTools doesn't make that time any longer.) 😄

@karlhorky
Copy link
Contributor Author

Yep, I don't mean to say it does take longer. The UX of standard OS browser tooltips is also not good.

My point is that there is low discoverability here, because so many other things are well designed in the Devtools. This is hostile not only to experienced developers, but moreso to those who need it more, beginners.

@karlhorky
Copy link
Contributor Author

I guess the best would be to just not truncate the value, if you're not going to visibly surface it elsewhere. But I guess you have a reason for not doing that.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

Yes, although it's a bit involved.

The reason we truncate values is because of auto indentation which offers a pretty large UX bump for developers working on large apps (like in Facebook).

Auto indentation works better (or rather, avoids the undesirable behavior of shrinking the indentation size to 0) when the width of components is less. Width is determined by: length of component name, number of badges, and size of key.

I chose to favor indentation behavior, even at the cost of truncating large keys, because I think it's the more important UX. Also because I think large keys aren't very common, and/or that keyed components can often be identified in other ways (like using the element picker or the hover-on-highlight UX feature).

So given this decision, the question is: how best to truncate? I could write a JavaScript solution that avoids truncated unless necessary, but this can get a bit complicated given the way our auto indentation calculation works- and adds overhead to the very performance-sensitive phase of scrolling. So I made the decision to go with a CSS based max-width.

It's not ideal in all cases. Certainly not in the small one your example app shows. But I think it's the right trade off for the most common cases, given the above considerations. 😄

@karlhorky
Copy link
Contributor Author

a JavaScript solution that avoids truncated unless necessary

Yeah, that doesn't sound great either. Complex and potentially a maintenance burden.

It's not ideal in all cases
I think it's the right trade off for the most common cases

Hmm... what I'm suggesting is that this can be improved without needing to affect the solutions for other constraints though. In other words, why can't we have both good indentation and good UX for showing keys?

Here are a couple of options:

  1. add to props panel on the right - in a section after a light divider (potentially also with a question mark that shows an explanation about the fact that things in this section are not really props)
  2. add another panel on the right for other things that are non-props

Other, less desirable options:

  1. have an icon inline which, when clicked, shows the full value out of the flow of the document

These are just off the top of my head. With more brainstorming, even more design options with good UX can be found.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

I'm not a fan of an inline clickable icon.

I feel less strongly about the other two proposals, although I also think this is fine as is to be honest 😄 I haven't heard any other complaints about the feature, so it isn't super high priority from my POV (when compared to many other more pressing issues or bugs).

You're welcome to submit a PR with a proposal if you would like though?

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 22, 2020

Ok, I would consider doing a PR for these (probably on the weekend).

If I move proposal 1 to the description of this issue, can we re-open it? Just in case anyone else gets around to it before I do?

@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

To be clear, I'm not committing to merge this. I am happy to review and consider it though.

I'm not going to re-open the issue as it currently stands because I disagree with the description and summary of this as a bug. It's a carefully considered design decision 😄 If you want to reword it to be more about investigating an alternate UX like you've proposed, then I'm willing to re-open it.

@karlhorky karlhorky changed the title Bug (React Devtools): Key value very difficult to find / use Improve UX of finding full key value Apr 22, 2020
@karlhorky
Copy link
Contributor Author

Right, sorry if this came off as aggressive or something - I didn't see a GitHub issue template for anything other than Bug or Security Vulnerability, so I thought those were the only options.

I've re-worded, does it sound better now?

@karlhorky karlhorky changed the title Improve UX of finding full key value Improve UX of finding full key value Apr 22, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Apr 22, 2020

No worries. 😄

Sure, I'll reopen it now.

FWIW, I think showing the key above Props, in a separate panel, would probably be less confusing than showing it in the props (since it's not on the props object and editing it wouldn't have any impact).

@bvaughn bvaughn reopened this Apr 22, 2020
@bvaughn bvaughn added Type: Discussion Type: Feature Request and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 22, 2020
@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 24, 2020

@bvaughn ok, I may have some time on the weekend to work on this.

Something similar to the image below (from facebook/react-devtools#328) be ok for you?

@gaearon
Copy link
Collaborator

gaearon commented Apr 24, 2020

Looks good

@bvaughn
Copy link
Contributor

bvaughn commented Apr 24, 2020

Sure, something like that 😄

@bvaughn
Copy link
Contributor

bvaughn commented Apr 28, 2020

Sebastian shared an interesting insight about this:

The JSX syntax gives the wrong mental model about this. It leads to poor intuitions about keys: Such as can a key change due to an update? Why can't I read the key from inside a component? Why can't a component set its own key?

The change being proposed here could reinforce this, since the workflow is to first select a component and then look at its key (rather than the other way around- looking at a key to see which Component is in its slot).

Seems like the design of this, if we do choose to add "key" to the right panel somewhere, should reinforce the right mental model. (The key needs to look like it owns the component, rather than the other way around. It should not look like just another prop.)

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 29, 2020

The key needs to look like it owns the component

Hm, interesting idea... teaching this mental model via the design may not be practical from a design or user expectations perspective though. Making the interface more confusing will not teach people anything, it will just make Devtools harder to use.

Putting the key visually "above" the component somehow would be pretty confusing, if a user clicked on the component. The user would expect to see the component itself, not the key first.

Other approaches that don't have key higher in the visual hierarchy may... work? Maybe going back closer to the original design by making key look less like an object key and live on the same level as props (remove the colon, bold the text). Not super enthusiastic about it, but maybe it would at least achieve the further distancing from Props that you're looking for?

@karlhorky
Copy link
Contributor Author

@bvaughn any news? Maybe we can merge as it is and re-think async afterwards?

@bvaughn
Copy link
Contributor

bvaughn commented May 11, 2020

Sorry for the delay. If there were updates, they would be on the issue or PR 🙂

I haven't thought of a UI treatment for this that I'm happy with yet (although admittedly it also hasn't been my top priority the past few days).

I don't think this is a critical bug fix that we need to land now and find a better solution later. It's a UI tweak, so we can take our time to get it right. I'll try to look at it some more this week.

@karlhorky
Copy link
Contributor Author

Ok, thanks for the update.

@karlhorky
Copy link
Contributor Author

Nice, cool solution for all of the constraints in #18737 (comment).

Thanks for your effort and attention seeing this through! 💯🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants