Inline code block support #43
Conversation
…ed for displaying inline code-blocks. Similar to a <pre> tag which supports customizeable inline background color and border.
|
sorry I would mantain the commit history, it's easier to understand what hapened and why we did it if we mantain that, can you create a PR from the original branch? |
|
Hey Andre, I don't see any particular benefit of having commit history from the previous branch, for this particular case. I have been flipping things around, along the way. While I can do that if you insist, I propose we "do nothing" or keep things cleaner and split commits into logical sequence:
In any case I will put this on hold and give Rajat an opportunity to fully test, report all issues he found in one batch or approve this PR before putting any extra effort. Let me know what you think, |
|
Getting this error while running for Hermes on iOS. Should I try the suggestion about or do something else? |
|
Please do. I was hesitant to push |
|
But in which folder? I am using the automated script and not sure where... |
|
|
|
I still get the same result on iOS. @azimgd can you please drop your ENV values? Could it be due to the old Xcode version(13.2.1)? I don't think so but maybe. Also, what is your cocoapods version? May be I am installing the wrong pods. It is worth noting that I am building the app on a simulator. |
What error do you get ? Could you send me a build log in collapsed format please. Does that only fail with Hermes or JSC as well ? Here is the build status I get: |
|
I was not clear. I didn't that I am getting errors. I tested with JSC and the build succeeded. But the UI had the same issue where none of the code blocks were visible. Thanks for the logs. I will compare them. On the other hand, the build failed with Hermes. I will post the logs tomorrow. For now, I have started the OS upgrade. |
|
Tested on XCode 13.2.1(13C100) all works fine. Can you confirm you are testing this branch without any code modifications ?
|
|
Trying... |
|
Ok, Now it worked. Maybe a cache issue. Thanks for the help. It's too late for me but the good news is that I can finally run my tests. I will give you the full report tomorrow and we can close this PR. I will keep reporting bugs separately but you can wait until all are listed. I will ping you for the same. |
|
BUG: iOS: Strange wrapping got long words. Steps:
Check the cursor in this image, the wrapping started at This is different from the Android
|
|
BUG: Android: Border looks a bit off. Steps: Check the second Text element in the code. The bottom border is very light compared to other edges. Source
|
|
BUG: Android: Code block overlapping with nested normal text. Steps: Check the Text element with the link (5 Text element). Previous text is overlapping with code block. Source
|
|
Question: How can we add a little gap between the lines so that the code block does not overlap? |
|
BUG: Android: Spacing issue at the starting edge of the code block. Steps: Check the 3 Text element in the source below. There is a space before the code block starts(i.e. after example word) and there is another space in the starting of code block. But the space after example is not rendered. Not present on iOS Source
|
|
BUG: Android: iOS: Border radius of starting edge is not applied in code blocks nested in paragraphs. Steps: Check the 4 Text element in the source below. There is no border-radius on the starting edge of the code block containing a link. Source
|
|
BUG: Android: Vertical alignment is not correct. Android seems to have more space above the text inside code block. Steps: Check the 2 Text element in the source below. Compare both Android and iOS. iOS looks good but android seems to be shifted a little bit downwards inside the code block area. Source
|
|
I think these are the bugs. cc: @azimgd |
|
All the above tests have run on the JSC dev build. I will test Hermes Builds now. Please use the following styles for |
|
Getting this error on iOS Hermes build. Any help @azimgd? |
Not a bug:Validate that this still happens by removing
Zoom-in emulator, that's a visual glitch rather than a bug.
I assume you mention this particular overlapping, not a bug. You missed Should be this: with some text nested is deep with code block.
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlock={textCodeBlock}>
https://github.com/Expensify/App/issues/13922#issuecomment-1433354811.
</Text>
Not supported. Out of scope.
All spaces given in your example are rendering correctly, make sure you have updated your code: <Text style={{lineHeight: 20, fontSize: 15, marginBottom: 40}}>
Inline text example
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlock={textCodeBlock}>
This text.
</Text>
</Text>Can't spot any shifting. Check the lower end of Clean build, checkout to base branch, rebuild. If base branch builds successfully report here, if not report to Rory. To be fixed:Agree. Might be. |
|
|
||
| NS_ASSUME_NONNULL_BEGIN | ||
|
|
||
| @interface RCTTextCodeBlockStyle : NSLayoutManager |
There was a problem hiding this comment.
What's the reason for using custom LayoutManager?
There are already other attributes like background colour and they don't require the change.
I guess we want to merge it later upstream so I think it makes sense to avoid introducing new mechanisms if they are not needed. However, I'm not sure we need it while writing this comment :)
There was a problem hiding this comment.
Looks that it's a standard approach https://levelup.gitconnected.com/background-with-rounded-corners-in-uitextview-1c095c708d14
| NSString *borderColor = [textCodeBlockStyle objectForKey:@"borderColor"]; | ||
| float borderRadius = [[textCodeBlockStyle objectForKey:@"borderRadius"] floatValue]; | ||
| float borderWidth = [[textCodeBlockStyle objectForKey:@"borderWidth"] floatValue]; | ||
| float horizontalOffset = 5; |
| float borderRadius = [[textCodeBlockStyle objectForKey:@"borderRadius"] floatValue]; | ||
| float borderWidth = [[textCodeBlockStyle objectForKey:@"borderWidth"] floatValue]; | ||
| float horizontalOffset = 5; | ||
| float verticalOffset = 2; |
| enclosingRect.size.height - (borderWidth * 2) | ||
| ); | ||
|
|
||
| UIBezierPath *path = [UIBezierPath bezierPathWithRoundedRect:resultRect byRoundingCorners:corners cornerRadii:CGSizeMake(borderRadius, borderRadius)]; |
There was a problem hiding this comment.
Should we set those values here?
context.setAllowsAntialiasing(true)
context.setShouldAntialias(true)
|
Just checked iOS side and it looks solid 🎉 ^ our approach |
@shawnborton WDYT? |
|
Had the same issues with running the app on iOS Hermes as @parasharrajat , tested the others and LGTM 👍 |
|
I'm testing Android and see problem with layout: |
|
on iOS it works fine. I guess android adds a padding without actually moving the component. |
@azimgd Maybe we should use ReplacementSpan to change size properly? |
Correct. It should not have border-left or border-right for these cases. It wasn't the case when I tested but It might be due to new changes. |
|
Hello, I'm coming from this issue and PR, and was also testing this PR together with @TMisiukiewicz, and I found some issues exclusively in Android. Source<Text style={{lineHeight: 20, fontSize: 15, color: 'red'}}>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
<Text
style={{color: '#fff', fontSize: 13}}
textCodeBlockStyle={textCodeBlockStyle}>
Inline
</Text>
Text
</Text>
|
|
@fabioh8010 Do you mind explaining the issues here? |
|
Hi @parasharrajat , we noticed that this PR could solve the back tick issue I have been working some time ago and I decided to do some tests with inline code support in this PR, and came across this issue in Android. During my tests, everything seems fine with iOS, but Android have some alignment problems as you can see in the screenshoot. |
|
Yeah I think I agree that we shouldn't draw the borders on the sides when wrapping multi-line. |
|
@azimgd Please look into the above requests. |
|
I'm back, on this issue right now. |
|
issue is on hold from my side until 6883 is completed. |
|
@azimgd Any updates? |
|
Could we also add support for padding as a nested style attribute here? We'll need it for mentions. |
|
Any update @azimgd? |
|
I have been working on a new approach, for which I expect to have an update by Monday. |
Upstream PR Link
Summary
New
textCodeBlockprop for a component which is used for displaying inline code-blocks. Similar to a<pre>tag which supports customizable inline background color and border.See Libraries/Text/TextCodeBlock.js for the list of available props.
See packages/rn-tester/js/examples/Text/TextExample.ios.js for a usage example.
Changelog
[iOS] [Added] - textCodeBlock prop for component
[Android] [Added] - textCodeBlock prop for component
Test Plan
<Text textCodeBlockStyle>should have be displayed inline, with a background color and a border around it.