Skip to content
This repository was archived by the owner on Jun 23, 2020. It is now read-only.

Comments

Update README's instructions#283

Merged
mahmoud-adam85 merged 3 commits intoghostery:masterfrom
remusao:update-readme
Apr 10, 2019
Merged

Update README's instructions#283
mahmoud-adam85 merged 3 commits intoghostery:masterfrom
remusao:update-readme

Conversation

@remusao
Copy link
Contributor

@remusao remusao commented Apr 9, 2019

This PR updates README.md with updated instructions to build the project from scratch. @pavel-cliqz thanks for your help with this, could you maybe have a look and check everything I changed makes sense?

Notes for testing this patch

Ideally the instructions should be enough to start from a fresh install of MacOS Mojave and build the project successfully.

@pavel-cliqz
Copy link
Contributor

Looks good to me, the only part I think, there is one final step missing, which will be to run 'npm run postinstall' command to rename some wrong import in react native.

@remusao
Copy link
Contributor Author

remusao commented Apr 9, 2019

@pavel-cliqz I think the postinstall step might only be required because with Node.js 11 the npm ci fails, thus postinstall does not run automatically. I've added it anyway just to make sure, it cannot hurt.

README.md Outdated
This branch is for mainline development.

This branch only works with Xcode 9.3 and supports iOS 10, and 11.
This branch only works with Xcode 10.1 and supports iOS 10, and 11.
Copy link
Contributor

Choose a reason for hiding this comment

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

supports ios 10, 11 and 12

Copy link
Contributor

Choose a reason for hiding this comment

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

small comment that I found, not a big deal, but maybe make sense to change. other then that looks great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. As per Naira's comment I changed it into "11 and above".

Copy link
Contributor

@naira-cliqz naira-cliqz left a comment

Choose a reason for hiding this comment

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

Other than mentioned comments it looks fine for me as well. Thank you @pavel-cliqz and @remusao !

README.md Outdated
This branch is for mainline development.

This branch only works with Xcode 9.3 and supports iOS 10, and 11.
This branch only works with Xcode 10.1 and supports iOS 10, and 11.
Copy link
Contributor

Choose a reason for hiding this comment

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

It actually support ios 11 and above. Could you please fix this line

Copy link
Contributor

Choose a reason for hiding this comment

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

@naira-cliqz 11 or 10.3? The deployment target i see is 10.3

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another deploment target for the target itself which is 11, and it overwrites project target. Project target comes from FF and we didn't change it to avoid one more merge conflict

```shell
sh ./bootstrap.sh
npm ci
npm run bundle-lumen
Copy link
Contributor

Choose a reason for hiding this comment

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

here we can add 2 cause for Lumen bundle-lumen and for Ghostery bundle-ghostery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a second command. Thanks for the suggestion.

@remusao
Copy link
Contributor Author

remusao commented Apr 10, 2019

@naira-cliqz @pavel-cliqz Thanks for the review. I updated the PR accordingly.

@mahmoud-adam85 mahmoud-adam85 dismissed naira-cliqz’s stale review April 10, 2019 12:30

changes already applied

@mahmoud-adam85 mahmoud-adam85 merged commit ae4b108 into ghostery:master Apr 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants