Details

Reviewers

tomek
atul
• abosh
kamil
ashoat

Commits

rCOMM3b80dea6799f: [desktop] Basic electron app

Summary

This diff adds a basic desktop app setup.

it's contained in the desktop workspace
desktop/pages contains code for the splash/ loading window and connection error window
desktop/forge.config.js is a configuration for electron-forge whish is a packaging helper

Test Plan

Run yarn dev in keyserver and web. Run yarn dev in desktop and check if it works in developement
Run yarn make and check if resulting executable works with production server

Look for any differences with the web browser version.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

michal added inline comments.Nov 16 2022, 6:43 AM

desktop/main.js
39 ↗	(On Diff #18422)	It makes it so we don't create another electron window when a user clicks a link (the link is opened in the external browser with the use of `shell.openExternal(url);`).

Responded to some of the review, left comments for the rest

Harbormaster failed remote builds in B13481: Diff 18481!Nov 16 2022, 9:02 AM

I think it's important to have Flow / ESM support in desktop, and Webpack sounds like the way to do it. Can you create a task and link here before landing?
I'm okay with sequencing that after landing this diff. We should prioritize the monthly goal... ideally we get Webpack working before the end of the month, but if not I'd rather have the app shipped without Webpack than not shipped

desktop/main.js
30 ↗	(On Diff #18481)	`openURL`, please try to match conventions in the codebase
1–2 ↗	(On Diff #18422)	Why can't we suffix with `.cjs` though?
19 ↗	(On Diff #18422)	If we were using Webpack we could load it using CSS modules
32 ↗	(On Diff #18422)	The line does not appear to have been removed
39 ↗	(On Diff #18422)	Please add a code comment explaining
87 ↗	(On Diff #18422)	Don't think this was addressed
185–186 ↗	(On Diff #18422)	@michal can you ping @atul to get him to respond here?
docs/dev_environment.md
643 ↗	(On Diff #18481)	Please match existing conventions. We don't say "And" here, we say "Next, open a new terminal and run:" in other sections. These docs have extremely high standards of review and you will save everybody some time if you pay deep attention
657 ↗	(On Diff #18481)	Tons of typos here...

This revision now requires changes to proceed.Nov 16 2022, 7:41 PM

Rename to .cjs, documentation changes and other fixes

Harbormaster failed remote builds in B13522: Diff 18531!Nov 17 2022, 9:32 AM

Here's the linear webpack issue, I'm going to focus on it whenever I have the time.

Also, @atul do you know why keyserver CI is failing on my desktop diffs? I've tried cloning a fresh repo and running yarn cleaninstall but I don't have any issues locally.

desktop/main.js
1–2 ↗	(On Diff #18422)	Sorry, I forgot to address this comment in particular. There's no problem with this if we decide to keep CJS for now, so I'm going to rename the files!
19 ↗	(On Diff #18422)	I'm going to mention this in the issue.
32 ↗	(On Diff #18422)	Sorry, I must have missed it, thank you for noticing
87 ↗	(On Diff #18422)	Yes, sorry, I was waiting for the decision, in the other inline comments, about removing the second use of `show`.
185–186 ↗	(On Diff #18422)	@atul what do you think?
desktop/package.json
6 ↗	(On Diff #18422)	Added `"type": "commonjs"`

Keyserver build will work if you add a COPY line here for your new workspace. Otherwise looks great!

This revision now requires changes to proceed.Nov 17 2022, 10:00 AM

Setting @atul to blocking so he responds to the question in the inline comment (not the CI question, that one has been resolved)

In D5629#168194, @ashoat wrote:

Setting @atul to blocking so he responds to the question in the inline comment (not the CI question, that one has been resolved)

Sorry, literally just saw the comments tagging me

atul added inline comments.Nov 17 2022, 10:19 AM

desktop/main.js
185–186 ↗	(On Diff #18422)	Yeah I think we should "keep the app running" since that's the behavior I'd except on macOS. My understanding is: Red close button in top left corner = close window CMD - Q or right-click "Quit" = close application

ashoat added inline comments.Nov 17 2022, 10:22 AM

desktop/main.js
185–186 ↗	(On Diff #18422)	But doesn't that behavior only apply to apps that support multiple windows? Eg. consider the Settings app, which closes when you close the window.

michal added inline comments.Nov 17 2022, 10:31 AM

desktop/main.js
185–186 ↗	(On Diff #18422)	On the other hand, Slack and the Calendar app also don't quit when you close their windows.

atul added inline comments.Nov 17 2022, 10:32 AM

desktop/main.js
185–186 ↗	(On Diff #18422)	Maybe? Not sure if these are all "single window" applications but sample of Apple first-party Calendar -> remains open News -> remains open Maps -> remains open Dictionary -> application closes Settings -> application closes Reminders -> application closes So loosely it seems like "utility" applications close when the close button is hit? Worth noting that Messages and Discord stay open when the close button is hit, so feels safe to match their behavior. I don't feel strongly, but my intuition is that leaving the application running would be more inline with what I'd expect.

Okay fair, thanks for the research @atul. At this point the only thing I'm waiting on is the fix to the keyserver build and we should be ready to go!

Fix keyserver build

ashoat requested changes to this revision.Nov 17 2022, 1:03 PM

ashoat added inline comments.

keyserver/Dockerfile
25–36 ↗	(On Diff #18548)	Why did you touch this?

This revision now requires changes to proceed.Nov 17 2022, 1:03 PM

Revert formatting. Sorry, I have format-on-save enabled so the changes were made automatically, but I should have noticed that.

Harbormaster completed remote builds in B13532: Diff 18548.Nov 17 2022, 1:17 PM

Harbormaster completed remote builds in B13533: Diff 18549.Nov 17 2022, 1:28 PM

atul added inline comments.Nov 17 2022, 3:54 PM

desktop/main.js
184–186 ↗	(On Diff #18422)	Looks like this is covered in the Electron docs:

atul attached a referenced file: F250871: a9f2c7.png. (Show Details)Nov 17 2022, 3:55 PM

Great!!

Left some minor nits/questions/suggestions, but nothing pressing.

(Doesn't matter a whole lot, but what do you think about renaming the desktop directory to electron?)

desktop/main.cjs
6 ↗	(On Diff #18549)	CC @ashoat @jon Are we going to have to change this to `localhost:3000/comm/` for those on Nix? Would be good to get everyone using port `3000` across the board regardless of mainline/nix dev environment to surface these issues ASAP + keep things consistent
14–61 ↗	(On Diff #18549)	This is a little hard to read (at least for me) with all the levels of indentation. Thoughts on pulling some of these "chunks" out into their own variables?
63–92 ↗	(On Diff #18549)	For future diffs it would be super helpful to attach some screenshots to `Summary`/`Test Plan`/etc to show reviewers how things look Another thing that would be "nice to have" for future diffs are links to the relevant docs. Especially for Electron because I think we can assume most reviewers aren't super familiar with the API (I might be wrong with that assumption though)
68–69 ↗	(On Diff #18549)	Would be helpful to have some sort of screenshot or visual to show how the app looks at these minimum dimensions
71 ↗	(On Diff #18549)	Are these the default values for `trafficLightPosition`? Correct me if I'm wrong, but I'd assume we only need to specify the `trafficLightPosition` if we're overriding the defaults?
72 ↗	(On Diff #18549)	Super minor nit, but we usually capitalize hex values so it'd look something like backgroundColor: '#0A0A0A'
73–75 ↗	(On Diff #18549)	I've noticed a lot of Electron apps (eg VS Code, Linear, etc) leave the Developer Tools thing in the menu even on "prod"... maybe we could keep them around as well? Might be helpful at some point when debugging and doesn't seem like it'd hurt? (Discord does not leave them in)
desktop/pages/error.html
13 ↗	(On Diff #18549)	We usually capitalize hex values in the codebase so this would be something like: <meta name="theme-color" content="#B91D47" />
desktop/pages/splash.html
13 ↗	(On Diff #18549)	We usually capitalize hex values in the codebase so this would be something like: <meta name="theme-color" content="#B91D47" />
desktop/pages/style.css
6 ↗	(On Diff #18549)	Can we pull hex values from `web/theme.css`? Noticed we're able to pull the `var(--font-stack)` CSS variable... can we pull CSS variables from `theme.css` as well? Feel free to disregard if it's not possible or I'm missing context on something

ashoat added inline comments.Nov 17 2022, 5:23 PM

desktop/main.cjs
6 ↗	(On Diff #18549)	Yup, the goal is to get everybody off of Apache. We have a detailed plan in ENG-2260

tomek accepted this revision.Nov 18 2022, 5:57 AM

This revision is now accepted and ready to land.Nov 18 2022, 5:57 AM

Small changes.

michal requested review of this revision.Nov 21 2022, 2:34 AM

michal added 1 blocking reviewer(s): atul.

michal added inline comments.

desktop/main.cjs
63–92 ↗	(On Diff #18549)	That's fair, I should have at least linked to the issue on Linear with pictures.
68–69 ↗	(On Diff #18549)	We could probably make the minimum a bit smaller but it's starting to look weird.
71 ↗	(On Diff #18549)	We have `titleBarStyle: 'hidden'` so by default it looks like this: I have moved them closer to the middle.
73–75 ↗	(On Diff #18549)	Sure
desktop/pages/style.css
6 ↗	(On Diff #18549)	This is the color for the dark blue that the Comm logo has in the background. It doesn't have a css variable in `theme.css`. Maybe I could add it as `--logo-bg` and use it here?

michal attached a referenced file: F254594: image.png. (Show Details)Nov 21 2022, 2:34 AM

michal attached a referenced file: F254592: image.png. (Show Details)

Harbormaster completed remote builds in B13606: Diff 18635.Nov 21 2022, 2:46 AM

Resigning since I accepted already and the re-request review seems targetted at @atul

Looks good! Accepting to unblock but let's add the --logo-bg CSS variable before landing

desktop/pages/style.css
6 ↗	(On Diff #18549)	I think that makes sense!

This revision is now accepted and ready to land.Nov 21 2022, 3:16 PM

Add --logo-bg css variable.

Harbormaster completed remote builds in B13657: Diff 18713.Nov 22 2022, 3:31 AM

Closed by commit rCOMM3b80dea6799f: [desktop] Basic electron app (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyNov 23 2022, 6:09 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMM3b80dea6799f: [desktop] Basic electron app.

[desktop] Basic electron app
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 18635

.gitignore

desktop/.eslintrc.json

desktop/forge.config.cjs

desktop/icons/icon.icns

desktop/icons/png/512x512.png

desktop/main.cjs

desktop/package.json

desktop/pages/error.html

desktop/pages/splash.html

desktop/pages/style.css

desktop/scrollbar.css

docs/dev_environment.md

keyserver/Dockerfile

package.json

yarn.lock

[desktop] Basic electron appClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 18635

.gitignore

desktop/.eslintrc.json

desktop/forge.config.cjs

desktop/icons/icon.icns

desktop/icons/png/512x512.png

desktop/main.cjs

desktop/package.json

desktop/pages/error.html

desktop/pages/splash.html

desktop/pages/style.css

desktop/scrollbar.css

docs/dev_environment.md

keyserver/Dockerfile

package.json

yarn.lock

[desktop] Basic electron app
ClosedPublic
Actions

Revision Contents
Changeset List