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

Add JS-Libs to RNC #560

Merged
merged 40 commits into from
Oct 6, 2020
Merged

Add JS-Libs to RNC #560

merged 40 commits into from
Oct 6, 2020

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Sep 29, 2020 •
edited
Loading

cc @Jag96 @AndrewGable

Fixes: https://github.com/Expensify/Expensify/issues/141828

Tests (Web, Desktop, iOS, Android):

  1. Test leaving inline code block comments with single back ticks
  2. Test fenced code block comments with triple back ticks
  3. Verify they have a monospaced font

@marcaaron marcaaron self-assigned this Sep 29, 2020
@marcaaron
Copy link
Contributor Author

Still working on this as code blocks aren't rendering correctly. But bumping the priority on this in my head as I think this important for general engineering adoption of Expensify Chat.

@marcaaron
Copy link
Contributor Author

Alright added some code block styles. Had a little trouble adding borders around Text elements on the native side if anyone has ideas on the best way to do that I searched around but struck out pretty hard. Also just using the default monospace font for now to get something cooking. We can always improve this as we go, but things look nice on desktop and I'd guess that's how most of us are using the app at this point.

Screenshots:

Web

Screenshot 1409

iOS

Screenshot 1408

Android

Screenshot 1407

@marcaaron marcaaron marked this pull request as ready for review September 30, 2020 03:33
@marcaaron marcaaron requested a review from tgolen as a code owner September 30, 2020 03:33
@shawnborton
Copy link
Contributor

One of our goals with RN is that we design it once and it looks the same everywhere, so personally I would love to keep pushing this so that we truly have the same style across web/desktop/mobile.

So to make sure I am understanding correctly, are you saying that the <Text> component can't take any border or padding or border radius on the mobile platforms?

@marcaaron
Copy link
Contributor Author

are you saying that the component can't take any border or padding or border radius on the mobile platforms?

I'm not sure I'd definitively say that, no. I just haven't found a way to make that happen yet. It seems like there is an inconsistency between behavior on native vs. web WRT the HTML rendering library we are using and asked if anyone has ideas for how to get this working.

@shawnborton
Copy link
Contributor

Cool cool. I'm not sure I fully understand the technical details here, but for something like this block here:
image

Couldn't we just wrap <Text> inside of a <View> (or create a custom component) to get the styling we need?

@marcaaron
Copy link
Contributor Author

Yeah I tried this already. It doesn't do what you'd expect. And does this weird thing:

Screenshot 1411

Here's what it looks like when we set a backgroundColor of #000 on the View and padding: 5 for example purposes...

Screenshot 1412

You can see a few things happening here...

  1. When the code tag is the only thing the View takes up the entire width of the screen
  2. When the code tag appears alongside regular text it gets "pushed up" for some reason

Maybe there's something else we can do?

@marcaaron
Copy link
Contributor Author

marcaaron commented Sep 30, 2020 •
edited
Loading

With some more tweaking I got us to here:

Screenshot 1413

But this still has the same problem of the single code blocks taking up the full width and still get squished up when next to other text.

@marcaaron
Copy link
Contributor Author

Oooh got a little closer by using a negative marginBottom

Screenshot 1415

Which just leaves the weird width issue...

@shawnborton
Copy link
Contributor

Can you try using alignSelf: 'flex-start' on the <View> that wraps the <Text>?

@marcaaron
Copy link
Contributor Author

I think that did it 🎉

Screenshot 1416

Can you explain to me why this worked? What about align-self influences the width of a flex child?

@shawnborton
Copy link
Contributor

I found this from digging around a SO post but I'm not entirely sure - my guess is that without it, the object just naturally wants to fill up the entire width of the container?

@shawnborton
Copy link
Contributor

Here is a font file we can use for the iOS/Android apps: https://www.dropbox.com/s/67t8dohjazk6zzv/GT-America-Exp-Mono-Regular.otf?dl=0

We're working on getting the correct font files/licensing from Grilli Type (cc @megankelso )

@AndrewGable
Copy link
Contributor

npm install is working now 👍

@marcaaron
Copy link
Contributor Author

Ok JS-Libs has been merged so this is off hold and ready for testing.

tgolen
tgolen previously approved these changes Oct 6, 2020
View reviewed changes
Copy link
Contributor

@tgolen tgolen 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, but you've got a conflict on the font file now

src/components/InlineCodeBlock/index.android.js Outdated Show resolved Hide resolved
src/components/InlineCodeBlock/index.ios.js Outdated Show resolved Hide resolved
src/components/InlineCodeBlock/index.js Outdated Show resolved Hide resolved
@marcaaron
Copy link
Contributor Author

Fixed the conflict and Updated!

@marcaaron
Copy link
Contributor Author

Updated

@tgolen tgolen merged commit de6e2f2 into master Oct 6, 2020
@tgolen tgolen deleted the marcaaron-expensimark branch October 6, 2020 21:36
This was referenced Oct 6, 2020
Merged
Merged
kidroca added a commit to kidroca/Expensify.cash that referenced this pull request Feb 19, 2022
@kidroca
Update NetInfo to v8
fc4a58c 
Regarding the breaking change - we're not using SSID information - it doesn't affect us

v8 Includes the following fixes:
8.0.0 (2022-02-10)

BREAKING CHANGES
it's possible this is a breaking change, depending on your app use case. If you relied on iOS SSID information and met Apple's strict criteria for accessing SSID, you need to set the new config value shouldFetchWiFiSSID to true. If you set it to true and do not meet the criteria your app may crash due to a memory leak. All versions prior to 7.1.12 would attempt to fetch the information regardless of permission, leak memory, and possibly crash. This change avoids that crash.

Bug Fixes
ios: avoid memory leak from ssid APIs by adding explicit config (Expensify#560) (fbf7c15), closes Expensify#420

7.1.11 (2022-02-08)
Bug Fixes
windows: fix race condition in WiFi connection details feature (Expensify#568) (0cd8132)

7.1.10 (2022-02-07)
Bug Fixes
android: use registerDefaultNetworkCallback so toggling airplane mode works (Expensify#571) (e8af2de)

7.1.9 (2022-01-26)
Bug Fixes
android: count native listeners / correctly disable listener if count == 0 (Expensify#569) (5ae16f6)

7.1.8 (2022-01-25)
Bug Fixes
windows: refactor implementation to avoid crashes (Expensify#564) (cc4bfa3)

7.1.7 (2021-12-20)
Bug Fixes
android: populate network value during initial module startup (Expensify#553) (c05080f)

7.1.6 (2021-12-13)
Bug Fixes
android: avoid send event when has no listener (Expensify#548) (cad47d8)

7.1.5 (2021-12-09)
Bug Fixes
android: use method-local ref to instance var for multi-thread safety Expensify#549 (Expensify#550) (81bbc87)

7.1.4 (2021-12-07)
Bug Fixes
android: try async state fetch as stale state workaround (Expensify#547) (937cf48), closes Expensify#542
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgolen tgolen tgolen approved these changes

@AndrewGable AndrewGable Awaiting requested review from AndrewGable

@Jag96 Jag96 Awaiting requested review from Jag96

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcaaron @shawnborton @AndrewGable @tgolen