-
Notifications
You must be signed in to change notification settings - Fork 33
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
Minor changes in Diagnostics view, RNIDE tabs and InspectDataMenu #548
Conversation
…re-mansion/react-native-ide into @p-malecki/minor-ui-fixes
…@p-malecki/minor-ui-fixes
This reverts commit 17bfe86.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some inline comments
export function isExpoProject() { | ||
// determine if a project is an Expo project based on dependencies and scripts in package.json | ||
const appRoot = getAppRootFolder(); | ||
const config = getLaunchConfiguration(); | ||
|
||
if (config.isExpo) { | ||
return true; | ||
} | ||
|
||
const packageJson = requireNoCache(path.join(appRoot, "package.json")); | ||
let hasExpoCLInDependencies = false; | ||
try { | ||
hasExpoCLInDependencies = | ||
Object.keys(packageJson.dependencies).some((dependency) => dependency === "expo") || | ||
Object.keys(packageJson.devDependencies).some((dependency) => dependency === "expo"); | ||
} catch (e) {} | ||
|
||
let hasExpoCommandsInScripts = false; | ||
try { | ||
hasExpoCommandsInScripts = Object.values<string>(packageJson.scripts).some((script: string) => | ||
script.includes("expo ") | ||
); | ||
} catch (e) {} | ||
return hasExpoCLInDependencies && hasExpoCommandsInScripts; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a more reliable mechanism for that in metro.ts
file called shouldUseExpoCLI()
const activeRNIDEColumn = window.tabGroups.all.find( | ||
(group) => group.activeTab?.label === "React Native IDE" | ||
)?.viewColumn; | ||
const column = activeRNIDEColumn === ViewColumn.One ? ViewColumn.Beside : ViewColumn.One; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what would happen if you always used Beside
? wouldn't it perform the logic for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation says about Beside
ViewColumn: A symbolic editor column representing the column to the side of the active one.
but that side seems to always be on the right of Radon so it doesn't work with two tabs setup where Radon is on the right and code on the left, nor vertical two tabs layout, because Beside
in those cases opens tab in new column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved but please adres inline comments
@@ -8,6 +8,7 @@ import { getAppRootFolder } from "../utilities/extensionContext"; | |||
import path from "path"; | |||
import { getIosSourceDir } from "../builders/buildIOS"; | |||
import { isExpoGoProject } from "../builders/expoGo"; | |||
import { shouldUseExpoCLI } from "../../src/project/metro"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this function to utils and import in booth places I don't see the reason for it to live i metro files specifically
This PR includes the following changes:
Diagnostics
viewInspectDataMenu
on smaller tabsgo home
button to always be enabledModal
screenshots: