-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[HOLD for payment 2024-04-09] Live Markdown Preview #27977
Comments
Triggered auto assignment to @bfitzexpensify ( |
Creating a new issue to track this as we're taking a different direction with this and the old issue is long. Currently - we're waiting for next month and having Tomasz Zawadzki from SWM take this up. I need to update the design doc with the new direction we've decided on |
Excited to see this one play out 🙂 |
Tomasz is back from OOO and is looking at the design doc. Asked for an update here |
Just wanted to confirm that we're investigating the feasibility of implementing this feature in React Native. At this point things look promising. I will keep you updated. |
@tomekzaw How are things looking? Do you think we could also get some regular updates here as well so we're all kept up with progress on this? |
@thienlnam Things are still looking promising, at least on iOS. Sorry for lack of updates last week, I had very little time to work on this due to lots of duties in other projects but at least I've managed to complete them so I can focus on this topic. Yesterday I was trying to dive into paragraph styling (e.g. for blockquotes or codeblocks) but I encountered some obstacles like retaining correct cursor position when typing. Because I'm building the PoC on top of React Native's At this point I'm pretty confident this feature can be done on iOS with some amount of native code. Now I would like to prepare a similar PoC on Android in order to validate the approach on this platform as well. There are some more questions and remarks I've gathered while working on this feature:
|
Thanks for the update!
What problems have they already solved that we can benefit from? Is it mainly the issues that stem from "the problems with asynchronous updates" with the native textInput? We actually initially went this direction (PR) but the PR was held up and we didn't get around to continuing with this. We can continue down that route, but I think it's worthwhile to explore what our current issues that make it difficult to do this natively.
Interesting, I agree we might end up with some unintended complexity if we end up converting it twice to add back in the syntax. I think the first solution, but worried that it will make the library harder to update in the future. I'm not entirely sure what this would look like though. The Objective-C parser is also pretty interesting and would definitely be a lot more performant. I think in the end it comes down to how much of a performance difference between the solutions and also maintainability. Currently I'm leaning towards the first solution as then we don't have to maintain another parser in native code but curious for your thoughts here
I think the discord format looks good, essentially make it look like the same as the other text within the markdown syntax. As for other questions, feel free to ask first in the expensify-swm channel if there are any questions I already have an answer for but for other things that may just be a matter of preference we might just need to start a conversation in expensify-open-source
Yeah this definitely doesn't look like a blocker for any means, and figuring out a workaround appears to be more headache than it is worth. I agree with your decision, let's just build this feature and then iterate on the spacing if we need to but that's not a problem at this point |
Definitely I don't think we'd want to use Objective-C or Swift – if we want to implement a native markdown parser I would strongly encourage that we build it with C/C++ so that it can be used across platforms. Furthermore, there's an existing md4c library that's very fast, has a wasm wrapper for use on web, and is compliant with the CommonMark standard. I'd be open to using that, but we have additional requirements:
I don't think we have such features in md4c, so that means we'd probably have to essentially rebuild Expensimark from scratch in C++. Overall that sounds like a lot of work when it's unclear whether Expensimark is a performance bottleneck for us, so I think the first option of just extending the JS implementation of Expensimark to meet our needs is the way to go for the V1 👍🏼 |
The way discord does it looks good 👍🏼
My advice is that you should just make a decision that you think is best and post it in slack. That way if anyone feels passionately they can have the chance to object, but you're not blocked on us. |
They look exactly the same to me 😅 let's not hack with extra spaces Edit: Oh, I see it now. I'd say let's not worry about the padding of inline code blocks on iOS as it's a platform limitation that doesn't really affect usability. Your iterative approach for addressing the platform limitations of |
question: does Aztec implement its own text renderer? |
I think they already solved some problems with native text formatting and selection but not sure if they also suffer from asynchronous updates, I would need to check that.
Thanks @thienlnam and @roryabraham for your feedback on this topic.
Could you please explain where these requirements come from? If we don't use react-native-aztec, do we need the HTML-formatted string? Currently, the iOS PoC that I'm working on does not need to use HTML, it parses Markdown directly to NSAttributedString.
I strongly agree with this approach, for now let's just use something that gives a correct result (maybe even for some subset of supported syntax, e.g. bold/italic/links), then measure its performance, see if it's a bottleneck and plan next steps.
I don't know yet but I can check it. Currently, I'm leaning towards not using react-native-aztec due to its complexity and the fact that it's another third-party dependency, and also needs to use HTML as the intermediate format. I think at some point we may face issues with text selection but they will be definitely easier to debug on a smaller codebase. |
Parsing the the HTML back into markdown is when we get to editing an existing message. Rory will have a better answer but I believe the second requirement is for rendering the html in the listview
Yup sounds great, let's do that! |
For updating this issue - here's the latest demo from @tomekzaw https://expensify.slack.com/archives/C04878MDF34/p1698167840008629 |
Update: Today I've implemented all formatting options on iOS (bold, italic, strikethrough, mention, link, email, inline code, codeblock, quote, heading). It feels quite robust and even the PoC implementation seems to be performant. There are some minor issues that need to be addressed (e.g. trailing whitespace or nested styles) but nothing critical.
How are messages stored in the database? I thought they are stored in Markdown and converted to HTML on the client side so that we can display them using
I've just realized that we still need some Obj-C logic to build Input (Markdown): Step 1. Convert Markdown to HTML using already existing ExpensiMark library. Output: Step 2. Parse HTML to common intermediate tree format. For instance, this could be JSON. This step is relatively easy since we can just find
Step 3. Convert tree in common intermediate format to platform-specific tree (i.e. This pipeline is already implemented in the PoC and seems to work fine. The presence of the intermediate format also makes it easy to inspect, debug and test. Also, it minimizes the amount of code that needs to be written separately for each platform. However, there's one problem with using ExpensiMark library for Markdown to HTML conversion in its current form (i.e. with removing Markdown syntax). Consider the following inputs:
Both of them give the following output: Another problem is related to whitespace handling and especially trailing spaces and newlines which are removed from the HTML output, for instance Some more questions: [Q1] All spaces in codeblocks are represented as [Q2] ExpensiMark library wasn't designed to handle Live Markdown Preview, so the following input: Sorry for long comment again and looking forward to your answers and remarks! |
They're stored as HTML in the database actually, so yeah agree that we can convert them into markdown and then use the obtained markdown for the TextInput
You bring up a pretty interesting point about the Markdown to HTML conversion with links. I agree it would be helpful if we had the existing markdown syntax in the output string but I'm not entirely sure we want to add that now - I think that those current edge cases seem fine. Like if we always restore markdown as a URL / add an extra space with blockquotes and it only happens when we edit a comment then I'd say that seems fine
👍
IIRC we decided that we would prevent any nested markdown within code blocks. So that is true - and I don't think we would want to support any formatting within a codeblock as normal markdown doesn't support it either
That could work, realistically I don't think anyone would be intentionally formatting a middle backtick in a code block. Though could we add or adjust a rule that just waits until it finds a resulting triple backtick? In any case, if that would overcomplicate things we can just go with your solution Thanks for the questions and updates! It's looking great so far |
Latest update here |
@tomekzaw, @thienlnam Eep! 4 days overdue now. Issues have feelings too... |
@tomekzaw, @thienlnam Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@tomekzaw, @thienlnam 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
We're close to re-enabling on web! Latest is here https://expensify.slack.com/archives/C06BDSWLDPB/p1708724096745109 |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.58-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-04-09. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO. |
We've been live! Now we're handling clean up in #36071 |
Proposal
https://expensify.slack.com/archives/CC7NECV4L/p1675122898707719
Design Doc: https://docs.google.com/document/d/1rcmXjgwvsM3MtdQnkJlMs6IS-i8SHJd0Vs7B_U0K3UU/edit
Proposal: Implement a Live Markdown Preview feature in NewDot
Context: Markdown is a text formatting syntax that we use in NewDot to add rich text formatting to messages. It allows users to add basic formatting such as bold, italic, bullet points, headings, links, etc. to their messages without having to use HTML or other complex formatting tools.
Problem:
As NewDot becomes more widely used for group communication, it’s essential to offer ways to effectively format and structure messages to improve productivity and avoid miscommunication similar to how we do resyncs in WN.
The problem is that without a visual representation of the final message, it can be difficult for users to confirm that their message is correctly formatted and conveys the intended information. Inconsistent or improperly formatted messages can hinder effective communication and decrease productivity, especially for users who are not familiar with markdown.
Solution:
To enhance user experience and improve communication, add a live markdown preview feature in the chat application. This will allow users to preview their message format in real-time and make any necessary adjustments before sending the message, resulting in clearer and more easily understood communications.
The live markdown preview feature is already present in Slack, and gives you the assurance that the message you are crafting, will show up exactly like you expect once sent.
The implementation will be done in collaboration with Callstack, utilizing the Design Doc process, and will be executed as a concurrent project. The live markdown preview feature is a front-end focused project and will not impact any internal employees or ongoing newsletters.
Tasks
#expensify-open-source
[email protected]
and paste in the Proposal[email protected]
(continue the same email chain as before) with the link to your Design Doc#expensify-open-source
to discuss any necessary details in public before filling out the High-level of proposed solution section.[email protected]
again with links to the doc and pre-design conversation in SlackDesignDocReview
label to get the High-level of proposed solution section reviewed#expensify-open-source
to ask for engineering feedback on the technical solution.DesignDocReview
label to this issue[email protected]
one last time to let them know the Design Doc is moving into the implementation phase[email protected]
once everything has been implemented and do a Project Wrap-Up retrospective that provides:The text was updated successfully, but these errors were encountered: