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.

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

Upgrade expensify-common #3106

Merged
merged 1 commit into from
May 25, 2021
Merged

Upgrade expensify-common #3106

merged 1 commit into from
May 25, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented May 25, 2021

Details

Made some changes to ExpensiMark that were recently merged and deployed to npm, so this PR upgrades expensify-common so we can have those new changes.

If you're confused as to why the package.json isn't using the commit hash anymore, it's because I followed these steps (which I think use the major/minor version): https://stackoverflow.com/c/expensify/questions/7216

Fixed Issues

N/A, but here's the expensify-common PR: Expensify/expensify-common#376

Tests

Same as QA

QA Steps

Try to send this as a message in any chat, and verify it renders the same as the screenshot:

```hi```
```hi
```
```
hi```
```
hi
```

Screen Shot 2021-05-24 at 8 38 41 PM

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

2021-05-24_19-20-04.mp4

Mobile Web

2021-05-24_19-42-48.mp4

iOS

2021-05-24_19-40-34.mp4

Android

1621913590738637.mp4

Desktop

2021-05-24_19-23-36.mp4

@jasperhuangg jasperhuangg self-assigned this May 25, 2021
"react-native-modal": "^11.10.0",
"react-native-onyx": "git+https://github.com/Expensify/react-native-onyx.git#d0a5d12c08f2f029e3038fb3516a93a9403e9746",
Copy link
Contributor Author

@jasperhuangg jasperhuangg May 25, 2021

Choose a reason for hiding this comment

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

CLARIFICATION: The version here wasn't changed (the commit hashes are the same), npm install simply changed the location of this when I ran it (I guess it sorted everything alphabetically).

@jasperhuangg jasperhuangg marked this pull request as ready for review May 25, 2021 02:45
@jasperhuangg jasperhuangg requested a review from a team as a code owner May 25, 2021 02:45
@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team May 25, 2021 02:45
@jasperhuangg
Copy link
Contributor Author

@Luke9389 I self-reviewed just to clarify some stuff, this should be good for a quick review from you, thanks!

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

Nice, simple version bump 👍

@Luke9389 Luke9389 merged commit c72ed63 into main May 25, 2021
@Luke9389 Luke9389 deleted the jasper-updateExpensiMark branch May 25, 2021 18:04
@@ -59,7 +59,7 @@
"electron-log": "^4.2.4",
"electron-serve": "^1.0.0",
"electron-updater": "^4.3.4",
"expensify-common": "git+https://github.com/Expensify/expensify-common.git#2e5cff552cf132da90a3fb9756e6b4fb6ae7b40c",
"expensify-common": "^1.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to revert this. Sorry @jasperhuangg the SO instructions you followed are outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @marcaaron, can you add context on why this is being reverted? Just curious to see what broke.

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be publishing expensify-common to npm. The advice in the SO is outdated (my bad). We must reference the commit hash instead like we have been doing for every other expensify-common update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants