-
Notifications
You must be signed in to change notification settings - Fork 135
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
PR for desktop app #1299
base: main
Are you sure you want to change the base?
PR for desktop app #1299
Conversation
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.
Awesome job 🎉 ! I only reviewed the frontend
folder. I'll take a look later to desktopApp
frontend/src/basic/Main.tsx
Outdated
@@ -10,7 +10,7 @@ import Notifications from '../components/Notifications'; | |||
import { useTranslation } from 'react-i18next'; | |||
import { GarageContext, type UseGarageStoreType } from '../contexts/GarageContext'; | |||
|
|||
const Router = window.NativeRobosats === undefined ? BrowserRouter : MemoryRouter; | |||
const Router = (window.NativeRobosats === undefined && window.DesktopRobosats === undefined)? BrowserRouter : window.DesktopRobosats === 'Desktop-App' ? HashRouter : MemoryRouter; |
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.
Not a fan of concatenated ternaries. Maybe you can solve it with a if..else or switch?
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.
okay
@@ -71,7 +71,7 @@ const RobotAvatar: React.FC<Props> = ({ | |||
|
|||
useEffect(() => { | |||
if (shortAlias !== undefined) { | |||
if (window.NativeRobosats === undefined) { | |||
if (window.NativeRobosats === undefined) { |
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.
Is it possible you didn't run linterns? usually the lint:fix
command will take care of these
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.
ohh i forget let me fix it
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.
Still present
frontend/src/contexts/AppContext.tsx
Outdated
@@ -40,6 +40,7 @@ export interface SlideDirection { | |||
export type TorStatus = 'ON' | 'STARTING' | 'STOPPING' | 'OFF'; | |||
|
|||
export const isNativeRoboSats = !(window.NativeRobosats === undefined); | |||
export const isDesktopRoboSats = !(window.DesktopRobosats === undefined); |
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.
Great idea, but you didn't use it bellow at :81
@@ -5,6 +5,7 @@ declare global { | |||
ReactNativeWebView?: ReactNativeWebView; | |||
NativeRobosats?: NativeRobosats; | |||
RobosatsSettings: 'web-basic' | 'web-pro' | 'selfhosted-basic' | 'selfhosted-pro'; | |||
DesktopRobosats: undefined | 'Desktop-App'; |
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.
maybe we can generalize this do it more similar to RobosatsSettings
, if we call this RobosatsClient: 'desktop-app'
will be more readable and I can do the same for Mobile in another PR, what do you think?
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 didn't understand
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.
Change this line for:
RobosatsClient: 'desktop-app' | undefined
And where you are checking in other parts for DesktopRobosats
switch it to RobosatsClient === 'desktop-app'
that way it's more readable and will help other platforms such as the mobile android to reuse it
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.
ok i guess then i need to change in html file also, to window.RobosatsClient = 'desktop-app'
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.
and replace every use of window.DesktopRobosats === 'Desktop-App' with RobosatsClient === 'desktop-app'
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.
That's correct
// make the textarea out of viewport | ||
textArea.style.position = 'fixed'; | ||
textArea.style.left = '-999999px'; | ||
textArea.style.top = '-999999px'; |
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.
Maybe you can achieve the same with just visibility = hidden;
?
@@ -17,4 +18,4 @@ export const systemClient: SystemClient = | |||
// react-native-web view of the RoboSats Android app. | |||
window.navigator.userAgent.includes('robosats') | |||
? new SystemNativeClient() | |||
: new SystemWebClient(); | |||
: window.navigator.userAgent.includes('Electron')? new SystemDesktopClient() : new SystemWebClient(); |
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.
Same about ternaries
@KoalaSat have a look now . |
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.
Awesome job with /desktopApp
! I just have one quick question, I see a lot of apparent duplication in tor/tor-*
:
- Are those README files necessary/different?
- Are the files under
/data
diferent per architecture? if not, maybe you can just move it up and use a symlink
desktopApp/app.js
Outdated
@@ -0,0 +1,142 @@ | |||
// Modules to control application life and create native browser window |
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.
Did you tried to change this file to Typescript?
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.
just ignore this file i am gonna delete this i have converted it in ts as index.ts
@@ -0,0 +1 @@ | |||
{"version":3,"file":"index.js","sourceRoot":"","sources":["index.ts"],"names":[],"mappings":";;AAAA,uEAAuE;AACvE,qCAAsE;AACtE,+CAAsE;AACtE,2BAA6B;AAC7B,uBAAyB;AAEzB,IAAI,GAAG,GAA0C,IAAI,CAAC;AAEtD,2EAA2E;AAE3E,SAAS,sBAAsB;IAC7B,IAAM,QAAQ,GAAG,EAAE,CAAC,QAAQ,EAAE,CAAC;IAE/B,QAAQ,QAAQ,EAAE,CAAC;QACjB,KAAK,OAAO;YACV,GAAG,GAAG,IAAA,qBAAK,EAAC,IAAI,CAAC,IAAI,CAAC,SAAS,EAAE,0BAA0B,CAAC,CAAC,CAAC;YAC9D,MAAM;QACR,KAAK,QAAQ;YACX,GAAG,GAAG,IAAA,qBAAK,EAAC,IAAI,CAAC,IAAI,CAAC,SAAS,EAAE,sBAAsB,CAAC,CAAC,CAAC;YAC1D,MAAM;QACR,KAAK,OAAO;YACV,GAAG,GAAG,IAAA,qBAAK,EAAC,IAAI,CAAC,IAAI,CAAC,SAAS,EAAE,wBAAwB,CAAC,CAAC,CAAC;YAC5D,MAAM;QACR;YACE,MAAM,IAAI,KAAK,CAAC,gCAAyB,QAAQ,CAAE,CAAC,CAAC;IACzD,CAAC;AACH,CAAC;AAED,gCAAgC;AAChC,sBAAsB,EAAE,CAAA;AAGxB,qCAAqC;AACrC,GAAG,CAAC,MAAM,CAAC,EAAE,CAAC,MAAM,EAAE,UAAC,IAAY;IACjC,IAAM,OAAO,GAAG,IAAI,CAAC,QAAQ,EAAE,CAAC;IAChC,OAAO,CAAC,GAAG,CAAC,yBAAkB,OAAO,CAAE,CAAC,CAAC;AAC3C,CAAC,CAAC,CAAC;AAEH,qCAAqC;AACrC,GAAG,CAAC,MAAM,CAAC,EAAE,CAAC,MAAM,EAAE,UAAC,IAAY;IACjC,OAAO,CAAC,KAAK,CAAC,0BAAmB,IAAI,CAAC,QAAQ,EAAE,CAAE,CAAC,CAAC;IACpD,cAAG,CAAC,IAAI,CAAC,CAAC,CAAC,CAAC,CAAC,sDAAsD;AACrE,CAAC,CAAC,CAAC;AAEH,iDAAiD;AACjD,SAAS,YAAY;IACnB,qDAAqD;IACrD,IAAM,UAAU,GAAG,IAAI,wBAAa,CAAC;QACnC,KAAK,EAAE,IAAI;QACX,MAAM,EAAE,GAAG;QACX,IAAI,EAAC,IAAI,CAAC,IAAI,CAAC,SAAS,EAAE,wCAAwC,CAAC;QACnE,cAAc,EAAE;YACd,eAAe,EAAE,KAAK,EAAG,8CAA8C;YACvE,gBAAgB,EAAE,IAAI,EAAG,wCAAwC;SAClE;KACF,CAAC,CAAC;IAEH,kDAAkD;IAClD,UAAU,CAAC,OAAO,CAAC,iBAAU,IAAI,CAAC,OAAO,CAAC,SAAS,EAAE,mBAAmB,CAAC,CAAE,EAAE;QAC3E,YAAY,EAAE,oBAAoB,CAAE,qCAAqC;KAC1E,CAAC,CAAC;IAEH,oDAAoD;IACpD,UAAU,CAAC,WAAW,CAAC,EAAE,CAAC,eAAe,EAAE;QACzC,OAAO,CAAC,GAAG,CAAC,sCAAsC,CAAC,CAAC;QACpD,UAAU,CAAC,OAAO,CAAC,iBAAU,SAAS,uBAAoB,CAAC,CAAC;IAC9D,CAAC,CAAC,CAAC;IAEH,oDAAoD;IACpD,yCAAyC;AAC3C,CAAC;AAED,kEAAkE;AAClE,cAAG,CAAC,SAAS,EAAE,CAAC,IAAI,CAAC;IACnB,2CAA2C;IAC3C,YAAY,EAAE,CAAC;IAEf,2GAA2G;IAC3G,cAAG,CAAC,EAAE,CAAC,UAAU,EAAE;QACjB,IAAI,wBAAa,CAAC,aAAa,EAAE,CAAC,MAAM,KAAK,CAAC;YAAE,YAAY,EAAE,CAAC;IACjE,CAAC,CAAC,CAAC;AACL,CAAC,CAAC,CAAC;AAEH,+CAA+C;AAC/C,cAAG,CAAC,EAAE,CAAC,OAAO,EAAE;IACd,oCAAoC;IACpC,kBAAO,CAAC,cAAc,CAAC,UAAU,CAAC,eAAe,CAAC,EAAE,IAAI,EAAE,CAAC,kBAAkB,CAAC,EAAE,EAAE,UAAC,OAAO,EAAE,QAAQ;QAClG,IAAM,GAAG,GAAG,OAAO,CAAC,GAAG,CAAC;QACxB,IAAM,WAAW,GAAG,GAAG,CAAC,KAAK,CAAC,CAAC,CAAC,CAAC;QACjC,IAAM,cAAc,GAAG,IAAI,CAAC,IAAI,CAAC,SAAS,EAAE,WAAW,CAAC,CAAC;QACzD,QAAQ,CAAC,EAAE,WAAW,EAAE,iBAAU,cAAc,CAAE,EAAE,CAAC,CAAC;IACxD,CAAC,CAAC,CAAC;IAEH,qDAAqD;IACrD,kBAAO,CAAC,cAAc,CAAC,QAAQ,CAAC;QAC9B,UAAU,EAAE,wBAAwB;QACpC,gBAAgB,EAAE,SAAS;KAC5B,CAAC,CAAC;AACL,CAAC,CAAC,CAAC;AAEH,kDAAkD;AAClD,cAAG,CAAC,EAAE,CAAC,mBAAmB,EAAE;IAC1B,yCAAyC;IACzC,GAAG,aAAH,GAAG,uBAAH,GAAG,CAAE,IAAI,EAAE,CAAC;IACZ,IAAI,OAAO,CAAC,QAAQ,KAAK,QAAQ;QAAE,cAAG,CAAC,IAAI,EAAE,CAAC;AAChD,CAAC,CAAC,CAAC"} |
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.
Is this necesary?
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.
when i compile the ts file i got this file
slot?.hashId && | ||
!Boolean(robot?.lastOrderId) && | ||
!robot?.lastOrderId && |
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.
Any reason for these changes?
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.
This happens because I did lint:fix
@@ -71,7 +71,7 @@ const RobotAvatar: React.FC<Props> = ({ | |||
|
|||
useEffect(() => { | |||
if (shortAlias !== undefined) { | |||
if (window.NativeRobosats === undefined) { | |||
if (window.NativeRobosats === undefined) { |
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.
Still present
#1299 (comment) i thought i only need to change for window.desktopApp ? |
I wad talking ahout the blank space, if you ran |
I already run that don't know why it is there |
which readme file |
For example |
Yes Readme are same in all three tor/ |
Then maybe we can move it up to the parent folder and avoid duplicated files |
All good now good job with the duplications, I wanted to test it but I realized I have no idea how to run it 😄 can you add a |
When I try to test it in windows and clone it in windows but the symlink is not working |
@amitpanwar789 Tested on OSx and it looks great!! There is only one missing part on my opinion, did you configured the icon? |
for icon we need to make the icon type .ico for window and .icns for mac to configure it correctly |
You can create then with this one https://github.com/RoboSats/robosats/blob/main/nodeapp/assets/icon/Robosats.svg |
I have tested on linux and it working fine |
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.
Build tested in mac and it works! 🚀
desktopApp/Readme.md
Outdated
```bash | ||
npm run package:linux | ||
npm run package:win | ||
npm run package:mac |
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.
npm run package-mac
and I guess same for tohers
No description provided.