[QuickOpen] show indicator if symbols are still being parsed - #5371
Conversation
|
We don't have any spinners yet, so lets just re-use the "Loading..." message we have in some other places |
|
The spinner is just a temporary filler proof of concept. Where is the loading message used elsewhere? |
|
Thanks. I set it up the right way I think now (Visually). I mainly have to configure the logic to work exactly how it should, it's a bit wrong right now. |
|
The travis test is failing at the moment regarding the quick-open... can you take a look? do you have experience running mochi tests? Happy to help if needed! |
|
@codehag no I haven't got any experience yet using mochi tests |
|
looking better. How about we center the loading message and add some vertical padding |
|
@atwalg2 please share a screenshot |
|
|
| color: white; | ||
| } | ||
|
|
||
| .loadIndicator { |
There was a problem hiding this comment.
How about loading-indicator? CSS rules generally are not camel case
There was a problem hiding this comment.
I'll fix this right away
| render() { | ||
| const { enabled, query } = this.props; | ||
| const { selectedIndex, results } = this.state; | ||
| let { selectedIndex, results, isLoading } = this.state; |
There was a problem hiding this comment.
const -> let am I missing something here?
There was a problem hiding this comment.
agreeing with @AnshulMalik -- since none of these variables are reassigned (and since they are part of state and should only be updated via setState), we can use const here!
There was a problem hiding this comment.
I'll fix this right away
| if (query == "") { | ||
| return this.showTopSources(); | ||
| this.showTopSources(); | ||
| return this.setState({ isLoading: false }); |
There was a problem hiding this comment.
this.setState({ isLoading: false }); can be moved to the top of this function (since it always happens) so that we don't have it repeating in each if statement
There was a problem hiding this comment.
I'm glad you said this because it made me re examine my code, and realized I was causing wasteful re renders, I moved the isLoading: false to the instances when results is already being set through setState
| updateResults = (query: string) => { | ||
| if (this.isGotoQuery()) { | ||
| return; | ||
| return this.setState({ isLoading: false }); |
There was a problem hiding this comment.
hmm, re isLoading... i dont think we need it. Let me explain
the only time we're really loading is when symbols are not fetched. We can check this.props.symbols and if it doesn't have any functions it is still loading otherwise we're not
onChange and updateResults dont matter because it's instant
cdcd96b to
e674302
Compare
|
Need some advice on what is going on here. Travis Ci Error from test/mochitest/browser_dbg-quickopen |
|
hmm... i think that's happening for others |
|
do you need a rebase |
| selectedItemId={expanded ? items[selectedIndex].id : ""} | ||
| /> | ||
| {!symbols || | ||
| (symbols.functions.length == 0 && ( |
There was a problem hiding this comment.
sorry, this should be symbols.identifiers i think i told you the wrong thing
There was a problem hiding this comment.
These are the only properties of symbols??
|
Hello @atwalg2 ! It looks to me like the
|
| } | ||
| /> | ||
| {!symbols || | ||
| (symbols.functions.length == 0 && ( |
There was a problem hiding this comment.
This symbols.functions.length will always fail if there's no query text, so we should add that to this conditional.
There was a problem hiding this comment.
Also, when I do add text to the input, and there are many results, symbols.functions.length is still 0, so maybe we should be doing a different check?
|
We'll be updating the loading logic in #5588 so it's not a big deal |
Associated Issue: #4488
Summary of Changes
Test Plan
To test load a project into the debugger, then open up quick open using ctrl + o, and you should see the loading text appear, then disappear on it's own when the symbols have been parsed.