-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow HTML for task titles #55967
base: main
Are you sure you want to change the base?
Allow HTML for task titles #55967
Conversation
tests are because of a known broken test |
updated, please re-review |
nvm the default needs to change again, since its a string default we can't provide a value here |
ready for review! |
Reviewing the updated changes. Will have this finished by tomorrow. |
Let me know if you plan on getting to this today! |
Yes I'll get to this in an hour. |
Starting with this now. |
@johnmlee101 I found a couple of issues issue while testing. If I use a keyboard shortcut on description, the title ends up showing the HTML.
web-task-title.movweb-task-title-encoded.mov |
very odd, I'll take a look |
Hmm that's probably technically correct, but why doesn't the link use the larger headline style as well in your example? cc @Expensify/design |
I think it will need fixing. cc - @johnmlee101 |
Another question, with markdown now, do we support multiline title? Looks like in the current PR update task title supports multiline but create task doesn't. |
fixed! |
This gets into the styling overlap between markdown and task titles I'd imagine, not 100% sure what the problem is however and how to fix it |
Basically I would expect the entire title to look like our headline style, and the the linked portion is just blue but retains the same font size/boldness of the headline. Let me know if that makes sense, otherwise I am happy to mock something up. But basically just imaging having something like this: |
unfortunately |
Yeah this is the part I might need some redirection on where to modify this |
asking in opensource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only Text nodes that are direct children of Text nodes can inherit styles from parent nodes.
When the TNodeChildRenderer inside TaskTitleRenderer recursively renders AnchorRenderer, it seems that the direct Text child relationship is broken.
We can pass the style
to the Text node here in AnchorRenderer to fix this.
- style={styles.link}
+ style={[style, styles.link]}
@@ -111,7 +111,7 @@ function TaskView({report}: TaskViewProps) { | |||
numberOfLines={3} | |||
style={styles.taskTitleMenuItem} | |||
> | |||
{taskTitle} | |||
<RenderHTML html={`<task-title>${taskTitle}</task-title>`} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't found anything broken with this but react-native-render-html
warns against using RenderHTML
as a child inside Text
node.
Explanation of Change
This opens up being able to set and display html titles in plain text for most flows within the product. Pt. 2 of this by @Krishna2323 will add html rendering to the appropriate spots.
Fixed Issues
$ #53175
PROPOSAL: #53175 (comment)
Tests
<b>Title</b>
or<em>Title</em>
.Title
, no markdown.Offline tests
QA Steps
<b>Title</b>
or<em>Title</em>
.Title
, no markdown.PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps./** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-01-29.at.3.06.44.PM.mov
Screen.Recording.2025-01-30.at.2.51.08.PM.mov
MacOS: Desktop