Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update README for packaged app #392

Merged
merged 1 commit into from
Nov 5, 2021
Merged

Update README for packaged app #392

merged 1 commit into from
Nov 5, 2021

Conversation

arfio
Copy link
Contributor

@arfio arfio commented Jun 14, 2021

Add instructions on how to download and use the app.

Related to #352 #387

Motivation: Currently, users need to compile the Trace Viewer from source to view their own traces. To avoid the time-consuming dev setup and compilation we now offer a downloadable package (electron desktop application + trace server).

Signed-off-by: Arnaud Fiorini fiorini.arnaud@gmail.com
Signed-off-by: Erica Bugden erica.bugden@gmail.com

ebugden
ebugden approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks! ☀️

README.md Show resolved Hide resolved
README.md Outdated
@@ -106,11 +116,11 @@ For example, to start the browser example app with a specific URL, one can run
It's possible to package the repo's example application with `electron-builder`. After running `yarn` in the repo root, do:

```bash
$ cd electron-app
$ cd examples/electron
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to @arfio, both this correction and the correction below are required to make the existing packaging code work as expected. (These corrections are needed even if #387 is not merged as is.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to fix it in the other PR as well.

ebugden
ebugden approved these changes Jun 17, 2021
View reviewed changes
README.md Outdated
$ yarn package
```

The configured Linux packaging(s) will be generated folder `electron-app/dist`
The configured Linux packaging(s) will be generated folder `examples/electron/dist`
Copy link
Contributor

@ebugden ebugden Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the other required correction to have the existing packaging code work as expected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to fix it in the other PR as well.

ebugden
ebugden reviewed Jun 17, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
ebugden
ebugden suggested changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both @thefinaljob and @IbrahimFradj needed a hint to change the file permissions to executable so this requirement should be more clear.

README.md Outdated Show resolved Hide resolved
@arfio arfio force-pushed the packaged-app-doc branch from ef703d3 to 8af6c3a Compare June 17, 2021 21:34
@arfio arfio requested a review from ebugden June 17, 2021 21:38
@ebugden ebugden mentioned this pull request Jun 29, 2021
Closed
7 tasks
ebugden
ebugden previously approved these changes Jul 13, 2021
View reviewed changes
@ebugden
Copy link
Contributor

ebugden commented Jul 13, 2021 •
edited
Loading

Since #367 was merged the media file names have changed. This branch has not been rebased since then so the old file names are still there. Will merging this overwrite the file name changes made in #367? Or is only the diff shown in the "Commits" section applied to master when merged?

@ebugden
Copy link
Contributor

ebugden commented Jul 14, 2021

Since #367 was merged the media file names have changed. This branch has not been rebased since then so the old file names are still there. Will merging this overwrite the file name changes made in #367? Or is only the diff shown in the "Commits" section applied to master when merged?

Nevermind! I was misunderstanding how commits are represented and what action is actually performed when a PR is merged. This shouldn't be a problem.

@ebugden ebugden dismissed their stale review July 15, 2021 19:12

Need to specify "Java 11" before merging.

@arfio arfio force-pushed the packaged-app-doc branch from 8af6c3a to 0f86cc5 Compare July 23, 2021 15:06
ebugden
ebugden previously approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@ebugden ebugden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the update 🌳

bhufmann
bhufmann requested changes Jul 26, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
bhufmann
bhufmann reviewed Sep 24, 2021
View reviewed changes
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arfio, @ebugden I'll upload a PR to fix the lines for publishing to NPM.

README.md Outdated Show resolved Hide resolved
bhufmann
bhufmann previously approved these changes Nov 5, 2021
View reviewed changes
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the updates!

@bhufmann
Copy link
Collaborator

bhufmann commented Nov 5, 2021

@arfio please rebase the PR to the latest master so that I can merge it.

@arfio
Update README for packaged app
2a32a44 
Add instructions on how to download and use the app.

Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
Signed-off-by: Erica Bugden <erica.bugden@gmail.com>
bhufmann
bhufmann approved these changes Nov 5, 2021
View reviewed changes
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@bhufmann bhufmann merged commit 2681383 into eclipse-cdt-cloud:master Nov 5, 2021
@yoann-heitz yoann-heitz mentioned this pull request Nov 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebugden ebugden ebugden left review comments

@bhufmann bhufmann bhufmann approved these changes

@MatthewKhouzam MatthewKhouzam Awaiting requested review from MatthewKhouzam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arfio @ebugden @bhufmann