Selection scraper created and integrated to project #6732

kursat · 2021-12-13T14:41:02Z

Details

Fixed Issues

$ #5142

Tests & QA Steps

Android / IOS

Send some messages contains markdown.
Copy with context menu.
Ensure copied message is same with sent message.

Web / Mobile Web / Desktop

Send some messages contains markdown.
Copy with context menu.
Copy all message by selecting with mouse.
Copy single line of message by selecting with mouse/double clicking.
Copy a word of message by selecting with mouse/double clicking.
Copy multiple line of message by selecting with mouse.
Copy by selecting multiple messages with single/multiple lines. (This will not work as intended until another bug is fixed)
Ensure copied message is same with sent message.

Markdowns to Test

Code Block
Inline Code Block
Email
Link
Italic
Bold
Strikethrough
Quote
New line

Test String I've Used

    This is `code`
    ```
    this is block code
    ```
    Mail to: [Expensify]([email protected])
    Link: [Expensify](https://www.expensify.com)
    _italic_

    *bold*
    ~strikethrough~
    > Quote

Tested On

Screenshots

I'll upload IOS and Desktop videos ASAP.

Web

5142-web.mp4

Mobile Web

5142-mobile-web.mp4

Desktop

5142-desktop.mp4

iOS

5142-ios.mp4

Android

5142-android.mp4

thienlnam

Looks solid! Just a few formatting comments

thienlnam · 2021-12-14T18:02:54Z

src/libs/SelectionScraper/index.js

+const tagAttribute = 'data-testid';
+
+/**
+ * Reads html of selection. If there is no Selection API returns empty string.


If there is no selection, the API

I meant if browser doesn't support Selection API, Is it OK if I update it like that?

Hi @thienlnam, I've updated all comments, can you recheck, if they are OK?

Yes that update is good 👍 (just missing one more comment I left unresolved)

src/libs/SelectionScraper/index.js

thienlnam · 2021-12-14T18:07:13Z

src/libs/SelectionScraper/index.js

+                    } else {
+                        node = range.commonAncestorContainer.parentNode.closest(`[${tagAttribute}]`);
+                    }


When is this the case? / What does this else indicate

Still looking for this comment, on why the parentNode would have the closest tag attribute instead of the current node

src/libs/SelectionScraper/index.js

thienlnam

Comments look good, missing one comment and also looks like you have a large change with your package-lock.json that should be removed

thienlnam · 2021-12-15T18:26:13Z

src/libs/SelectionScraper/index.js

+                    } else {
+                        node = range.commonAncestorContainer.parentNode.closest(`[${tagAttribute}]`);
+                    }


Still looking for this comment, on why the parentNode would have the closest tag attribute instead of the current node

This reverts commit b7895a8.

kursat · 2021-12-20T23:39:06Z

Hey @thienlnam, sorry for the package-lock.json file, I removed it but now all tests are failing. Is this normal? Also added the comment you requested along with a bugfix for Chrome.

thienlnam · 2021-12-24T00:24:45Z

Sorry I should have clarified, package-lock.json file should be included but for some reason your package-lock.json diff seemed to be way too large to just include the new changes for packages you installed.

The tests are failing since you added new packages in package.json but the package-lock.json doesn't have them.

Could you try doing an npm install and then adding those changes again?

kursat · 2021-12-24T03:09:26Z

@thienlnam, I updated package-lock.json file. Can you check please?

It looks like using newer version of node.js or npm causes big changes on package-lock.json file.

kursat@kursat-laptop:~$ node -v
v14.18.1
kursat@kursat-laptop:~$ npm -v
8.2.0
kursat@kursat-laptop:~/WebstormProjects/upwork/ExpensifyApp$ npm i
npm WARN old lockfile 
npm WARN old lockfile The package-lock.json file was created with an old version of npm,
npm WARN old lockfile so supplemental metadata must be fetched from the registry.
npm WARN old lockfile 
npm WARN old lockfile This is a one-time fix-up, please be patient...

Can we add an engines section to package.json file to prevent other contributors to face the same problem?

thienlnam · 2022-01-03T18:52:19Z

Yup that looks much better, thanks!

Can we add an engines section to package.json file to prevent other contributors to face the same problem?

This is definitely something we can explore - feel free to propose the issue in the #expensify-open-source slack channel and then we can get the ball rolling from there

OSBotify · 2022-01-04T01:53:23Z

🚀 Deployed to staging by @thienlnam in version: 1.1.24-19 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

OSBotify · 2022-01-04T02:40:21Z

🚀 Deployed to production by @francoisl in version: 1.1.24-19 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

parasharrajat · 2023-11-13T16:08:58Z

A check contributed to the creation of invalid HTML tags when test-id attribute contains spaces. #29921. I am unable add the comment on the line.

Selection scraper created and integrated to project

ca3ebd8

kursat marked this pull request as ready for review December 13, 2021 15:37

kursat requested a review from a team as a code owner December 13, 2021 15:37

botify requested a review from thienlnam December 13, 2021 15:37

MelvinBot removed the request for review from a team December 13, 2021 15:37

thienlnam requested changes Dec 14, 2021

View reviewed changes

kursat added 2 commits December 15, 2021 12:56

comment update / code style

da87356

fixed package versions

b7895a8

thienlnam requested changes Dec 15, 2021

View reviewed changes

kursat added 4 commits December 21, 2021 00:52

Revert "fixed package versions"

be29b98

This reverts commit b7895a8.

removed package-lock.json changes, re-committing fixed versions.

c13d2cc

Fixed a bug which causes no newlines after comments

9d9ba2c

comment on selecting parentNode.closest()

95c6ceb

kursat requested a review from thienlnam December 22, 2021 19:29

thienlnam mentioned this pull request Dec 24, 2021

[HOLD for payment 2022-01-11] [$1000] Copying messages with markdown via CTRL + C does not keep formatting #5142

Closed

added package-lock.json (installed dependencies with lts/erbium)

41d7744

thienlnam approved these changes Jan 3, 2022

View reviewed changes

thienlnam merged commit 4151943 into Expensify:main Jan 3, 2022

OSBotify mentioned this pull request Jan 4, 2022

Deploy Checklist: New Expensify 2022-01-04 #7005

Closed

45 tasks

thienlnam mentioned this pull request Jan 24, 2022

[HOLD for payment 2022-05-20] [$2000] When copy&paste multiple messages line breaks aren't maintained #7352

Closed

tgolen mentioned this pull request Apr 21, 2022

Correct some improper code patterns #8733

Merged

parasharrajat mentioned this pull request Nov 13, 2023

[HOLD for payment 2023-11-09] [$500] Chat - The HTML code of the avatar is copied when copying the login name #29921

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selection scraper created and integrated to project #6732

Selection scraper created and integrated to project #6732

kursat commented Dec 13, 2021 •

edited

Loading

thienlnam left a comment

thienlnam Dec 14, 2021

kursat Dec 15, 2021

kursat Dec 15, 2021

thienlnam Dec 15, 2021

thienlnam Dec 14, 2021

thienlnam Dec 15, 2021

thienlnam left a comment

thienlnam Dec 15, 2021

kursat commented Dec 20, 2021

thienlnam commented Dec 24, 2021

kursat commented Dec 24, 2021

thienlnam commented Jan 3, 2022

OSBotify commented Jan 4, 2022

OSBotify commented Jan 4, 2022

parasharrajat commented Nov 13, 2023

Selection scraper created and integrated to project #6732

Selection scraper created and integrated to project #6732

Conversation

kursat commented Dec 13, 2021 • edited Loading

Details

Fixed Issues

Tests & QA Steps

Android / IOS

Web / Mobile Web / Desktop

Markdowns to Test

Test String I've Used

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam Dec 14, 2021

Choose a reason for hiding this comment

kursat Dec 15, 2021

Choose a reason for hiding this comment

kursat Dec 15, 2021

Choose a reason for hiding this comment

thienlnam Dec 15, 2021

Choose a reason for hiding this comment

thienlnam Dec 14, 2021

Choose a reason for hiding this comment

thienlnam Dec 15, 2021

Choose a reason for hiding this comment

thienlnam left a comment

Choose a reason for hiding this comment

thienlnam Dec 15, 2021

Choose a reason for hiding this comment

kursat commented Dec 20, 2021

thienlnam commented Dec 24, 2021

kursat commented Dec 24, 2021

thienlnam commented Jan 3, 2022

OSBotify commented Jan 4, 2022

OSBotify commented Jan 4, 2022

parasharrajat commented Nov 13, 2023

kursat commented Dec 13, 2021 •

edited

Loading