Open m-natarajan opened 6 months ago
Job added to Upwork: https://www.upwork.com/jobs/~01b55a4d576f4c3f8a
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 (External
)
Triggered auto assignment to @greg-schroeder (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Lack of maximum height for the description field for room hides other options on the page.
Here:
We are rendering full description with shouldRenderAsHTML
we should remove shouldRenderAsHTML
prop and add numberOfLinesTitle={4}
prop with appropriate maximum number of lines.
We can use windowWidth
to define dynamic maximum number of lines.
For lengthy descriptions, other options go out of the screen.
There is no height limit or lines limit in the below code in MenuItem
{!!title && (shouldRenderAsHTML || (shouldParseTitle && !!html.length)) && (
<View style={styles.renderHTMLTitle}>
<RenderHTML html={getProcessedTitle} />
</View>
)}
We can pass a prop from details page, which can be set as a max height limit in the above code to ensure that description height is limited.
It's not gonna be that easy guys :P context https://github.com/Expensify/App/pull/34150#issuecomment-1915928141 Seems we want a solve here for truncating html
We want to limit the number of lines for html content
Our migration from https://github.com/Expensify/App/pull/34150#issuecomment-1915928141 is anticipating markdown support. A choice was made during PR to go with no max lines for this HTML we are rendering as we do not have this feature yet.
I think as we start supporting more markdown/html we need more tooling to parse over this. We can leverage a library such as https://github.com/arendjr/text-clipper or implement our own to clip html
I also tried to solve it in a hackish way, setting a max height, overflowing, and adding ellipses with a pseudo element. It's ugly.
There is no maximum line for the report description.
Simply we don't limit the maximum line. The report description is rendered with a RenderHTML component and there is no straightforward way to set the number of lines props to the text. https://github.com/Expensify/App/blob/98461f987e08e1ee977bd60f4974513b84b3c93d/src/components/MenuItem.tsx#L515-L519
One way is to wrap the description with a new custom tag that will have a custom renderer that can receive a number of lines attribute.
function TextRenderer({TDefaultRenderer, textProps, ...props}) {
return <TDefaultRenderer {...props} textProps={{...textProps, numberOfLines: props.tnode.attributes.line}} />
}
text: TextRenderer,
and in BaseHTMLEngineProvider
text: HTMLElementModel.fromCustomModel({tagName: 'text', contentModel: HTMLContentModel.textual}),
MenuItem
, wrap the HTML with <text>
tag.
return processedTitle ? `<comment><text line="${numberOfLinesTitle}">${processedTitle}</text></comment>` : '';
ReportDetailsPage
, pass numberOfLinesTitle
with the agreed value
numberOfLinesTitle={3}
Why we can't create a custom renderer for <comment>
tag? That's because comment
is a block
model, not textual
which won't receive textProps
Full description displayed.
We are using RenderHTML
to display the discription. The component does not support clipping text.
https://github.com/Expensify/App/blob/dcf2e3e1ceebc8e598e6b51b61ebf76e297a2696/src/components/MenuItem.tsx#L517
Remove the option to render the description as HTML and just render as Text.
We can wrap the component with a Text and numberOfLines={1} prop.
+ <Text numberOfLines={1}>
<RenderHTML html={getProcessedTitle} />
+ </Text>
/>
When a room description is long, it pushes menu items off the screen so the user has to scroll down to see them. This happens on all platforms where the screen/window is too short to display everything in the details panel.
The room description's height is variable because it is determined by the length of the user input.
Move the room description below the menu items in ReportDetailsPage.tsx. This ensures the menu items are always visible and the layout stays consistent. The bottom of long descriptions will still be off the screen, but it will be visually obvious that the user can scroll down to see the full text.
This solution is very simple to implement and doesn't require any adding any tools, functionality, or conditions, so it won't cause regressions. However, if there are design docs or user manuals that show this screen, they will need to be updated.
📣 @sarahRosannaBusch! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Contributor details Your Expensify account email: annarose0113@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/sarahrosannabusch
✅ Contributor details stored successfully. Thank you for contributing to Expensify!
https://github.com/Expensify/App/issues/37357#issuecomment-1968568340 - @dragnoir sorry but i dont find you solution working as we expect i.e. render html as it is now just have a max height/ limit for number of lines
https://github.com/Expensify/App/issues/37357#issuecomment-1968221555 - @bernhardoj do you mind sharing a test branch(doesn't need to polished just a working POC)
https://github.com/Expensify/App/issues/37357#issuecomment-1968166026 - @ShridharGoel can you please elaborate where do you want to use the maxheight prop in codebase please give minimum details so it can be tested easily
https://github.com/Expensify/App/issues/37357#issuecomment-1967951200 - @rayane-djouah As per require we want to render the text as html not normal text
https://github.com/Expensify/App/issues/37357#issuecomment-1968190606 - @jeremy-croff i dont see a solution in your proposal but a library suggestion without any detail on how it can be integrated also if you want to propose a new library please follow this template for your proposal
@ishpaul777 sure, here is the test branch
Thanks for sharing the branch @bernhardoj I'll evaluate and test branch later today 🙇♂️
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@greg-schroeder, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
testing changes from @bernhardoj branch, will update asap
Unfortunately, the solution from @bernhardoj doesn't work for me at all
https://github.com/Expensify/App/assets/104348397/5f184adb-1eee-43b3-8643-ebb86e985106
Oh, you're right. I only test it with plain text 🤦
Waiting for proposals..
@ishpaul777 will you consider removing the option to render the description as HTML and just render as Text, as an option?
@ishpaul777 can you clarify what you're looking for here? If the requirement is to allow the user to input a long description that supports html/md, then we can't simply restrict the height of the description field because it is already a button. It would be very bad UX to have an "expand description" button within a button, which is why I proposed a simple layout change. Unless we don't need to show the full description on the details page and want to force the user to navigate to the Room Description editor to view the full text?
Hey there, @ishpaul777! I wanted to clarify my proposal for you. Basically, I suggested that we use either new tools or existing ones to parse HTML and truncate it. I'm not sure why this solution isn't being considered, but I hope we can discuss it further. Also, I provided a link to a library that can demonstrate how this can be done. Let me know if you have any questions!
My RCA also identified this as a new feature request.
The room description, when lengthy, can push other options off the screen. We need to truncate the description while preserving its Markdown nature and ensuring a good user experience.
The lack of truncation of the room description allows users to enter long descriptions, which can cause layout issues and push other important options off the screen. The description is rendered as Markdown, so any truncation solution needs to maintain the Markdown syntax.
truncateMarkdown
to truncate the Markdown text while preserving its syntax. We can also rely on a lib like this oneNote: We should add this type of truncation helper code to our ExpensifyMark lib and test thoroughly (can just use the code since it is written in vanillaJS and tweak for our purposes via review)
MenuItem
component, import the truncateMarkdown
function and use it to truncate the Markdown text before passing it to the RenderHTML
component:import ExpensiMark from '../lib/ExpensiMark';
const expensiMark = new ExpensiMark();
// ...
{!!title && (shouldRenderAsHTML || (shouldParseTitle && !!html.length)) && (
<View style={styles.renderHTMLTitle}>
<RenderHTML html={expensiMark.truncateMarkdown(getProcessedTitle, { limit: maxLength, ellipsis: true })} />
</View>
)}
With these modifications, the room description will be truncated to a specified maximum length (e.g., 100 characters) while maintaining the Markdown syntax.
https://github.com/Expensify/App/assets/15656774/e0b273b6-3c3c-41be-b3f4-ed8630520ce1
Note: It is important to note that even though we allow renderAsHTML
you cannot enter HTML into the input box when typing a description so in reality, we just need to truncate any text as if it was markdown
react-native-read-more-text
to handle the truncation, but this would add an additional dependency and may not work well with the RenderHTML component being inside a pressable button.The proposed solution of truncating the Markdown text directly, while preserving its syntax, provides a clean and efficient approach without relying on external libraries or modifying the rendered HTML.
Cc. @ishpaul777
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@greg-schroeder @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
@ishpaul777 is still going through proposal review
@ishpaul777 will you consider removing the option to render the description as HTML and just render as Text, as an option?
@dragnoir This will not be the correct solution and cause rendering plain html as text.
https://github.com/Expensify/App/assets/104348397/1b8af993-8d2b-4811-bd25-b09faaea4b44
@sarahRosannaBusch I dont consider your solution because the layout shift feels odd design(cc @Expensify/design please check screenshot from https://github.com/Expensify/App/issues/37357#issuecomment-1970096810 if that looks good..) and doesn't solve the root cause for issue but a workaround.
@jeremy-croff I like the idea of clipping the plain text before rendering as html, but adding a new library feel overkill to me for the issue. Can you add Plan of action in your proposal if you want to introduce a method to clip html without external library dependency
@brandonhenry Will you test your solution with all types markdown and refine it if needed share a test branch for testing purpose general idea seems a-okay!
@ishpaul777 If clipping the plain text is an idea you want to proceed with I can continue to write a POC of this implementation. I think, in general, my RCA was accurate by identifying the app changes and the decisions made that led to this bug.
Key points in what my intention was when suggesting "new tooling" or a library.
I think because of these factors I reserved some ambiguity as the exact util/architecture/package should require some internal review and I would proceed with some confirmation. But the linked library serves as a good pulse check on what the code would look like, and is about 600 lines to solve for this generically ( all elements ). I think an in-house solution should follow similiar logic.
The previous solution, from @brandonhenry, I think won't cut it because it's assuming its markdown is separate from "\n," but that's not true. And even then can only truncate a whole line at a time.
Thanks! :)
@ishpaul777 will setup a POC with a more robust markdown truncation 👍🏿
I think to correctly clip/truncate markdown in all cases of HTML it requires a robust implementation, hinting at a common utility or library, but we would foresee re-using this for more cases
Actually we do have some utilities https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js can we reuse any of utility from here ??
I think to correctly clip/truncate markdown in all cases of HTML it requires a robust implementation, hinting at a common utility or library, but we would foresee re-using this for more cases
Actually we do have some utilities https://github.com/Expensify/expensify-common/blob/main/lib/ExpensiMark.js can we reuse any of utility from here ??
This is exactly the one I was referring to for "3. It could be an extension of an existing common expensify package"
It seems we could reuse some of the regex for identifying the html blocks but still need an overal implementation for truncating.
I am reviewing https://github.com/Expensify/App/blob/main/contributingGuides/HOW_TO_CREATE_A_PLAN.md, is this what you want me to do?
@ishpaul777 good call. but as other contributor pointed out, there doesn't seem to be any markdown
truncation helpers in the ExpensifyMark lib. I do think we should update the lib with the truncateMarkdown
function as the solution.
See updated proposal
Actually before anyone invest efforts for the implementation lets confirm if truncation the of the text is right direction (design and UX wise)
@greg-schroeder for now we only have a acceptable idea that suggest to truncate the text for description. Should we move ahead with this idea?
there was also a good idea in https://github.com/Expensify/App/issues/32615 (where feature is implemented) but we were trying to move quickly so skipped this requirement.
Truncating the text makes sense to me - how else would we handle this? cc @Expensify/design
Also to clarify, I don't think we want a more control - we just want a truncated preview as a push row, and then clicking on the row would show a view that has the entire description. Pretty sure that's what @dubielzyk-expensify intended from the mocks at least.
Yep, exactly what Shawn said. Also want the truncating
what do you think @ishpaul777
what do you think @ishpaul777
I am in agreement also 👍
@ishpaul777 awesome! so how about my proposal? Any feedback?
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
@greg-schroeder @ishpaul777 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
@brandonhenry Do you mind sharing a test branch with your solution, with the https://github.com/pchiwan/markdown-truncate code modifed to adapt to our requirement.
@ishpaul777 sorry for delay, crazy week. I will have this test branch all ready to go before Monday!
Not overdue Waiting for test branch. also open for new proposals
@ishpaul777 fix is up on this PR. Uses the truncation method
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 1.4.44-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @JmillsExpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1709046310280799
Action Performed:
Expected Result:
All the option in the details page should be visible restricting the max height of the description field
Actual Result:
Lack of a max height for the description field obscures other options
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @greg-schroeder