Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Visual tweaks for the quick open drop-down. #3634

Merged
merged 10 commits into from
May 12, 2013
Merged

Conversation

larz0
Copy link
Member

@larz0 larz0 commented Apr 26, 2013

@njx pls review.

@ghost ghost assigned njx Apr 26, 2013
border-radius: 0 0 4px 4px;
box-shadow: 0 5px 10px rgba(0, 0, 0, 0.1);

// using this to fix the spacing that breaks item seperators and had to use !important to override inline css
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS comments use /* */. I'll push a fix for this if this is the only issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LESS supports // style line comments. The difference is that /* */ style comments in a LESS file are maintained in the generated .css file, where // style comments are stripped.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I had no idea. As Emily Litella would say, "Never mind."

@njx
Copy link

njx commented Apr 26, 2013

Hey--I like removing the separators, but there's some concern that it seems a bit hard to distinguish the individual list items from each other--it's a bit of a sea of text. A couple of things I tried:

  • lightening the path strings to #aaa - that helped, but made the paths a little too hard to read, and they're important when you have the same filename in multiple paths, like main.js or strings.js
  • adding a very light gray background (e.g. #f6f6f6) to even-numbered items - this made them stand out a bit too much

Would it be appropriate to make the background gray and add striping similar to your mockup for the Find in Files results? There the striping seems to work well without making either the lighter or darker rows stand out too much.

(BTW, I don't think we want to fix this by adding more padding between the items...want to keep the information density we have now.)

@larz0
Copy link
Member Author

larz0 commented Apr 29, 2013

np I'll give that a go.

@larz0
Copy link
Member Author

larz0 commented Apr 29, 2013

Updated the branch.

@jasonsanjose
Copy link
Member

I like the new look a lot! My only concern is that the selected color doesn't stand out enough.

@adrocknaphobia
Copy link
Contributor

I'm OK with the subdued selected color. The selection is interaction based (moving the arrow keys or hovering w/ a mouse). So the movement and interaction draw your attention to the highlight. If there was no movement then I'd agree w/ @jasonsanjose, but I think it works here.

.quicksearch-namematch {
font-weight: bold;
font-weight: 500;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use @font-weight-semibold

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use @font-weight-semibold

@peterflynn
Copy link
Member

Re removing the scroller shadow: what about when there's a ModalBar open at the top (e.g. Find)? Or a permanent menu bar (in inBrowser mode)? Do we still need a shadow for it to look good in those cases?

@larz0
Copy link
Member Author

larz0 commented Apr 30, 2013

@peterflynn ModalBars always sits on top of content so it will always have a shadow, so now only CSS is being used instead of JS. How do I go into inBrowser mode?

@peterflynn
Copy link
Member

Oh, I didn't see that you'd added a fixed shadow to ModalBar -- great! That same rule also covers inBrowser mode (that's what #titlebar is), so I think you're all set.

@njx
Copy link

njx commented May 4, 2013

@larz0 - I think we ended up still having concerns about the selection highlight being too subtle. The issue is that (because of the way our widget works currently) you might start off with something other than the topmost item being highlighted (because if the mouse happens to be over the list when it opens up, the item underneath it gets selected even if you don't click on it). This probably isn't ideal, but fixing it would be difficult until we get a better widget implementation.

So, it would be better if we could have a slightly colored highlight that was easier to see immediately against the striped items.

@larz0
Copy link
Member Author

larz0 commented May 4, 2013

No problem, try it now.

@njx
Copy link

njx commented May 12, 2013

Looks good--I merged it manually with master since there was a recent conflict.

njx pushed a commit that referenced this pull request May 12, 2013
Visual tweaks for the quick open drop-down.
@njx njx merged commit d6522da into master May 12, 2013
@njx njx deleted the larz/quick-open-tweaks branch May 12, 2013 22:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants