Follow up improvements for inline code blocks on native devices#79815
Follow up improvements for inline code blocks on native devices#79815QichenZhu wants to merge 9 commits intoExpensify:mainfrom
inline code blocks on native devices#79815Conversation
|
|
|
Oh sorry. I see that you wanna split these up. Ignore me. |
|
Looks like a good first step! Can you show a before/after screenshot too for easy comparison of the changes? |
|
@dubielzyk-expensify @shawnborton, thanks for your review. Below shows before and after comparisons. A couple things to note:
|
|
Awesome, this is looking good to me 👍 Can you show some examples that use letters with descenders, like |
|
Okay cool, so it feels like we generally have more background color above the letterforms than below. Is there a way to shift things around where we drop the bg color down by a pixel? Like if the top had a margin of 3px, but the bottom had a margin of 1px instead of 2px on both. Let me know if that makes sense. |
|
The last option there feels the best to me, where we basically have capital letters perfectly centered within the space. cc @Expensify/design for a gut check on that though. |
|
Yup I'm with you - bottom option is looking pretty good! |
|
Fair shout, @QichenZhu can we center it even more? |
|
That seems pretty good to me. Lowercase letters will always feel like they sit slightly down, but I think that's fine. Let's roll with it and see how it feels. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ef88ee82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - attributes[NSBackgroundColorAttributeName] = RCTEffectiveBackgroundColorFromTextAttributes(textAttributes); | ||
| + UIColor *bgColor = RCTEffectiveBackgroundColorFromTextAttributes(textAttributes); | ||
| + if (!isnan(textAttributes.borderTopLeftRadius) && textAttributes.borderTopLeftRadius > 0) { | ||
| + attributes[RCTTextBackgroundBorderRadiusAttributeName] = @[@(textAttributes.borderTopLeftRadius), bgColor]; |
There was a problem hiding this comment.
Guard custom attribute array against nil background colors
This branch can execute when opacity is set even if backgroundColor is missing (if (textAttributes.backgroundColor || !isnan(textAttributes.opacity))), and in that case bgColor may be nil; constructing @[@(textAttributes.borderTopLeftRadius), bgColor] with a nil element raises an Objective-C exception and can crash iOS text rendering for styles that combine borderTopLeftRadius and opacity without a background color. Please guard bgColor before creating the array (or fall back to the existing background-color path).
Useful? React with 👍 / 👎.
|
Hi @rojiphil, could you review this when you get a chance? |
|
@rojiphil, this is ready for your review. |
|
@rojiphil, this PR is prone to conflicts but they won't affect the review. I'll resolve them weekly or on request. Thanks! |
|
@QichenZhu I tested the PR but I am unable to see the rounded corners. Am I missing something here?
79815-ios-standalone-001.mp4 |
|
@rojiphil it could be the remote cache. Could you try rebuilding the iOS app locally or requesting an ad hoc build? |
|
Hi @rojiphil, how's the review going? Were you able to rebuild locally or request an ad hoc build? |












Explanation of Change
This implements step 1 on iOS of the 3-step incremental approach:
Fixed Issues
$ #57556
PROPOSAL:
Tests
Note: This change only affects iOS Native.
Offline tests
Same as Tests.
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native (N/A)
Android: mWeb Chrome (N/A)
iOS: Native
iOS: mWeb Safari (N/A)
MacOS: Chrome / Safari (N/A)