Display React logo only on active tab#4779
Conversation
|
This has room for improvement beyond just fixing the bug (I'd prefer to have framework logos on the appropriate tabs all the time like in a code editor) but I've been familiar with the project for about 30 minutes, so I only did the bug for now :D |
|
@fivepointseven I see what you're saying! And i see how you fixed the problem, which is a step in the right direction. I'd like to take a stab at showing the correct logo for all the tabs though. I'm traveling, so i'm a bit less responsive, but I bet you can try getting the source meta data for all the tabs. It's a bit inefficient the way we're doing it now, but we can refactor it later. |
|
@jasonLaster I tried doing that yesterday and came up with a bunch of different ways to do it actually, but the problem is that for some reason |
|
@jasonLaster okay, done :) |
codehag
left a comment
There was a problem hiding this comment.
looks good, one comment and a question
| const sourceTabs = getSourcesForTabs(state); | ||
| const sourceTabsMetaData = {}; | ||
| sourceTabs.forEach(source => { | ||
| const sourceId = source ? source.get("id") : ""; |
There was a problem hiding this comment.
im wondering, what is the case when a source does not have an id? do you have an example?
also, meta thought: it feels like the meta data belongs to the tab as a data structure too..
There was a problem hiding this comment.
This was copied from the latest code, so I'm not entirely sure:
- const selectedSource = getSelectedSource(state);
- const sourceId = selectedSource ? selectedSource.get("id") : "";
+ sourceTabs.forEach(source => {
+ const sourceId = source ? source.get("id") : "";Is there a case where source doesn't have an id?
A funny thing about metadata btw, it's set on "LOAD_SOURCE_TEXT", which means that in order for isReactComponent to be set tab needs to be visited at least once (if you open a few tabs and reload the window, logos won't show up until you click on respective tabs). It doesn't affect UX that much, and when I tried moving it to reducer it disintegrated my CPU, so I probably didn't entirely understand how it works outside of Tabs component, so I didn't touch it.
There was a problem hiding this comment.
@fivepointseven that's a good observation. We should probably re-think source metadata so it is a field in sources and tabs, so that when we re-show a tab on reload it is saved...
that way you don't need to revisit it.
There was a problem hiding this comment.
@jasonLaster What's the starting point in source's "life"? Cause I tried adding that at ADD_SOURCES action but it ended up eating all of my resources over 10 seconds after loading so I probably did it wrong.
There was a problem hiding this comment.
haha - yeah. we don't want to parse every source! it would kill us :)
don't worry about that bug now, I'll add some context in another issue
There was a problem hiding this comment.
Okay, cool then. So, do you have any issues with this PR I should fix? :)
| endPanelCollapsed: boolean, | ||
| searchOn: boolean, | ||
| sourceMetaData: SourceMetaDataMap | ||
| sourceTabsMetaData: any |
There was a problem hiding this comment.
hmmm perhaps we can make a type annotation?
| importObj => | ||
| importObj.source === "react" && | ||
| importObj.specifiers.some(specifier => specifier === "React") | ||
| ) || identifiers.some(identifier => identifier.name === "React") |
There was a problem hiding this comment.
this is an interesting change. Why is this needed? Also, could we add some new tests that would fail w/o it
There was a problem hiding this comment.
I bet this change could be in its own PR actually.
There was a problem hiding this comment.
This is what caused the problem I mentioned earlier:
for some reason isReactComponent was false the whole evening for me even with the current code
import React was expected here and it didn't trigger by const React = require('react'), so I added the identifier check as well
There was a problem hiding this comment.
Ahh brilliant. Lets do this in a separate PR. It fixes another problem :)
There was a problem hiding this comment.
Oh, didnt realize that
I just kinda did it cause it was interfering with what I was doing :D
There was a problem hiding this comment.
Yep! I do that all the time too :)
glad you got to see the parser code. It's my favorite part of the codebase
|
Minor point, when you open your next PR use a non master branch as it's easier for us to pull down and to push changes to https://github.com/devtools-html/debugger.html/blob/master/docs/pull-requests.md#git-workflow |
|
Just checked and it works nicely! Happy to land it after we discuss the parser change |
* Display React logo only on active tab * Properly display React logos for all open React tabs * Source tabs metadata type * Revert parser changes
Associated Issue: #4773
Summary of Changes
Screenshots/Videos (OPTIONAL)