Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ExpensiMark: Add New Markdown Truncation Utility #675

Merged
merged 44 commits into from
Jul 15, 2024
Merged

ExpensiMark: Add New Markdown Truncation Utility #675

merged 44 commits into from
Jul 15, 2024

Conversation

brandonhenry
Copy link
Contributor

@brandonhenry brandonhenry commented Apr 1, 2024 •
edited
Loading

Fixed Issues

$ Expensify/App#37357

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  2. What tests did you perform that validates your changed worked?

https://github.com/Expensify/expensify-common/blob/7c88b3ff10f92ab044fae7b4bac838b6d37fa1f0/__tests__/ExpensiMark-Markdown-Truncate-test.js

@brandonhenry brandonhenry requested a review from a team as a code owner April 1, 2024 16:41
@brandonhenry brandonhenry marked this pull request as draft April 1, 2024 16:41
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 1, 2024 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@melvin-bot melvin-bot bot requested review from NikkiWines and removed request for a team April 1, 2024 16:42
@brandonhenry
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@brandonhenry brandonhenry changed the title Add New Markdown Truncation Utility ExpensiMark: Add New Markdown Truncation Utility Apr 3, 2024
@brandonhenry brandonhenry marked this pull request as ready for review April 3, 2024 03:41
@brandonhenry
Copy link
Contributor Author

@NikkiWines this is ready for review!

@brandonhenry brandonhenry mentioned this pull request Apr 3, 2024
Closed
6 tasks
@brandonhenry
Copy link
Contributor Author

slight bump here @NikkiWines

@brandonhenry brandonhenry mentioned this pull request Apr 4, 2024
Merged
54 tasks
NikkiWines
NikkiWines requested changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a 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

lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
lib/ExpensiMark.js Outdated Show resolved Hide resolved
@shubham1206agra
Copy link

@brandonhenry Please test this on every possible markdown.

@puneetlath
Copy link
Contributor

cc @shubham1206agra as C+

@brandonhenry
Copy link
Contributor Author

This is ready to go with tests included

Cc. @shubham1206agra @NikkiWines

@brandonhenry
Copy link
Contributor Author

slight bump @NikkiWines @shubham1206agra

@shubham1206agra
Copy link

@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).

@brandonhenry
Copy link
Contributor Author

@shubham1206agra @NikkiWines all patched up here, ready for final review

@NikkiWines
Copy link
Contributor

Will take a look at this first thing tomorrow, sorry for the delay

@brandonhenry
Copy link
Contributor Author

@NikkiWines no worries, any updates here?

NikkiWines
NikkiWines requested changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple small things

__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-Markdown-Truncate-test.js Show resolved Hide resolved
lib/ExpensiMark.ts Outdated Show resolved Hide resolved
@brandonhenry
Copy link
Contributor Author

@NikkiWines thanks!! applying fixes now

@brandonhenry
Copy link
Contributor Author

Sorry got a little backed up fixing some typing issues on another expensify ticket. Should get these today

@brandonhenry
Copy link
Contributor Author

Thoughts here? @NikkiWines made adjustments! SOrry for delay

NikkiWines
NikkiWines requested changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a 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

__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
@brandonhenry
Copy link
Contributor Author

@NikkiWines updates made!

NikkiWines
NikkiWines requested changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a 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!

__tests__/ExpensiMark-Markdown-Truncate-test.js Outdated Show resolved Hide resolved
@brandonhenry
Copy link
Contributor Author

@NikkiWines last note patched!

Cc. @shubham1206agra

@brandonhenry
Copy link
Contributor Author

@NikkiWines lemme know how this looks

NikkiWines
NikkiWines requested changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a 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

__tests__/ExpensiMark-Markdown-Truncate-test.js Show resolved Hide resolved
__tests__/ExpensiMark-Markdown-Truncate-test.js Show resolved Hide resolved
@brandonhenry
Copy link
Contributor Author

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

@NikkiWines
Copy link
Contributor

Sorry to hear that @brandonhenry, take your time!!! Hope things are alright and services are restored soon 🍀

@brandonhenry
Copy link
Contributor Author

brandonhenry commented Jul 13, 2024 •
edited
Loading

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 🙏🏿

Cc. @NikkiWines @shubham1206agra

@brandonhenry
Copy link
Contributor Author

@NikkiWines got those last updates in, sorry about that! 🙃

NikkiWines
NikkiWines approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: looks good!

@puneetlath puneetlath merged commit cd42f4e into Expensify:main Jul 15, 2024
6 checks passed
@github-actions GitHub Actions
Copy link

🚀Published to npm in v2.0.44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubham1206agra shubham1206agra shubham1206agra left review comments

@NikkiWines NikkiWines NikkiWines approved these changes

@puneetlath puneetlath puneetlath approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@brandonhenry @shubham1206agra @puneetlath @NikkiWines