Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Style line numbers in search results#4541

Merged
wldcordeiro merged 2 commits into
firefox-devtools:masterfrom
princiya:search-results-remove-colon
Nov 2, 2017
Merged

Style line numbers in search results#4541
wldcordeiro merged 2 commits into
firefox-devtools:masterfrom
princiya:search-results-remove-colon

Conversation

@princiya

@princiya princiya commented Oct 31, 2017

Copy link
Copy Markdown
Contributor

Associated Issue: #4488

Summary of Changes

  • : is removed from the line numbers in search results

Before

screen shot 2017-10-31 at 13 28 09

After

screen shot 2017-10-31 at 13 25 56

@princiya

Copy link
Copy Markdown
Contributor Author

I have been trying to understand the file structure for the search component.

And, the : is coming from here.

What is the best way to drop this in ResultList?

  • Remove it from utils/quick-open.js?
    • There might be other dependencies involved here.
    • In other words, what is the motivation to add : in this file?
  • Parse subtitle to display only the number without the : here?

@jasonLaster

Copy link
Copy Markdown
Contributor

deferring to @wldcordeiro

@wldcordeiro

wldcordeiro commented Nov 1, 2017

Copy link
Copy Markdown
Contributor

I like the change. It looks nice! The CI failure is legit but it's an easy fix.

Change this line's assertion https://github.com/devtools-html/debugger.html/blob/master/src/test/mochitest/browser_dbg-quick-open.js#L102

// from
results.forEach(result => is(result.textContent, "x:13"));
// to
results.forEach(result => is(result.textContent, "x13"));

@wldcordeiro

Copy link
Copy Markdown
Contributor

Something to note. Atom formats their results for symbols like this, perhaps we can do this. Though it'd be a bit more of a change.

symbolName
Line 13

screen shot 2017-11-01 at 9 21 50 am

@jasonLaster

Copy link
Copy Markdown
Contributor

Hmm, i like that wellington, but lets hold off on that for now

VS Code is more compact and i think we should stick to one thing at a time

@wldcordeiro wldcordeiro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just need the tweak to the mochitest file mentioned. But ready to go!

@wldcordeiro wldcordeiro merged commit d152ca1 into firefox-devtools:master Nov 2, 2017
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.

3 participants