-
Notifications
You must be signed in to change notification settings - Fork 136
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
ExpensiMark: Add New Markdown Truncation Utility #675
ExpensiMark: Add New Markdown Truncation Utility #675
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@NikkiWines this is ready for review! |
slight bump here @NikkiWines |
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.
some very minor stylistic requests
@brandonhenry Please test this on every possible markdown. |
cc @shubham1206agra as C+ |
Co-authored-by: Nikki Wines <nikkiwines@expensify.com>
Co-authored-by: Nikki Wines <nikkiwines@expensify.com>
This is ready to go with tests included |
slight bump @NikkiWines @shubham1206agra |
@brandonhenry, I have reviewed your PR, and it seems wrong, as we have different sets of markdowns than those used here. I will suggest an alternative approach for you. Run a similar approach but on HTML tags, as they always have close tags (except for tags listed here). |
@shubham1206agra @NikkiWines all patched up here, ready for final review |
Will take a look at this first thing tomorrow, sorry for the delay |
@NikkiWines no worries, any updates here? |
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.
Couple small things
@NikkiWines thanks!! applying fixes now |
Sorry got a little backed up fixing some typing issues on another expensify ticket. Should get these today |
Co-authored-by: Nikki Wines <nikkiwines@expensify.com>
Thoughts here? @NikkiWines made adjustments! SOrry for delay |
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.
Couple more comments on the tests but looks pretty good
@NikkiWines updates made! |
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.
One last comment, looks good otherwise!
@NikkiWines last note patched! Cc. @shubham1206agra |
@NikkiWines lemme know how this looks |
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.
I see just a few more misleading test values but otherwise looks good
Hi @NikkiWines i am living in Houston and we got hit really bad by beryl. No power since Sunday night. Will keep updated as I also have no service so have to drive out and get it |
Sorry to hear that @brandonhenry, take your time!!! Hope things are alright and services are restored soon 🍀 |
Just got back online today! Catching back up and hope to wrap this up berfore EOD tomorrow. Thank you so much for the extra patience here - truly crazy times 🙏🏿 |
@NikkiWines got those last updates in, sorry about that! 🙃 |
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.
looks good!
🚀Published to npm in v2.0.44 |
Fixed Issues
$ Expensify/App#37357
Tests
https://github.com/Expensify/expensify-common/blob/7c88b3ff10f92ab044fae7b4bac838b6d37fa1f0/__tests__/ExpensiMark-Markdown-Truncate-test.js