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.

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

[Upload video API] Add forkid option and fix many issue when upload video #175

Merged
merged 6 commits into from
Jan 6, 2023
Merged

Conversation

hotrungnhan
Copy link
Contributor

@hotrungnhan hotrungnhan commented Dec 14, 2022

  • Add forkid option when upload
  • Reduce timeout when add video to the input
  • Fix upload limit can't detectable
  • remove default title when typing title and description

I only use upload video api, hope some body add these thing for edit metadata and other api....


This change is Reviewable

+ add forkid option when upload
+ reduce timeout when create video
+ fix upload limit can't detect
+ remove default title before assign new title
+ change when remove text func when typing title and description
+ upload dependencies
@guardrails
Copy link

guardrails bot commented Dec 14, 2022

⚠️ We detected 1 security issue in this pull request:

Vulnerable Libraries (1)
Severity Details
Low pkg:npm/node-fetch@2.6.7@2.6.7 (t) - no patch available

More info on how to fix Vulnerable Libraries in JavaScript.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

@hotrungnhan hotrungnhan changed the title Add forkid option (upload video) and fix many issue when upload video [upload video] Add forkid option and fix many issue when upload video Dec 14, 2022
@hotrungnhan hotrungnhan changed the title [upload video] Add forkid option and fix many issue when upload video [Upload video API] Add forkid option and fix many issue when upload video Dec 14, 2022
yarn.lock Outdated Show resolved Hide resolved
src/upload.ts Outdated Show resolved Hide resolved
@fawazahmed0
Copy link
Owner

@hotrungnhan Thanks for this PR, could you please reply to this and this .

+ add restriction option
src/types.ts Outdated Show resolved Hide resolved
renovate.json Outdated
Comment on lines 1 to 5
{
"extends": [
"config:base"
]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that file gone now ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert now

Comment on lines +533 to +547
// await page.keyboard.down('Control')
// await page.keyboard.press('A')
// await page.keyboard.up('Control')
// await page.keyboard.press('Backspace')
await textBoxes[0].evaluate(e => (e as any).__shady_native_textContent = "")
await textBoxes[0].type(title.substring(0, maxTitleLen))
}
// Edit the Description content (if)
if (description) {
await textBoxes[1].focus()
await page.keyboard.down('Control')
await page.keyboard.press('A')
await page.keyboard.up('Control')
await page.keyboard.press('Backspace')
// await textBoxes[1].focus()
// await page.keyboard.down('Control')
// await page.keyboard.press('A')
// await page.keyboard.up('Control')
// await page.keyboard.press('Backspace')
await textBoxes[1].evaluate(e => (e as any).__shady_native_textContent = "")
Copy link
Contributor

Choose a reason for hiding this comment

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

This code got commented.
Is that on purpose ?
Either code is useful and we keep it, either it's useless and we remove it and explain why.

Copy link
Contributor Author

@hotrungnhan hotrungnhan Jan 6, 2023

Choose a reason for hiding this comment

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

sorry, I forget to explain. That this shortcut unreasonably does not work to clear all the existing text (default video title). that must be replaced with the below
await textBoxes[1].evaluate(e => (e as any).__shady_native_textContent = "")

Copy link
Contributor

Choose a reason for hiding this comment

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

then we should prob delete the other lines rather than commenting them :P

@fawazahmed0
Copy link
Owner

@hotrungnhan , if you have tested this PR, please let me know. I will merge this PR.

@hotrungnhan
Copy link
Contributor Author

hotrungnhan commented Jan 1, 2023

@hotrungnhan , if you have tested this PR, please let me know. I will merge this PR.

Hold on i still not done. This is my commit just to update in my fork repo.
@pierreminiggio i will told you whenever it ok

@hotrungnhan
Copy link
Contributor Author

@pierreminiggio check it one more time and we can merge @fawazahmed0 .

@fawazahmed0
Copy link
Owner

Thanks

@fawazahmed0 fawazahmed0 merged commit a070b11 into fawazahmed0:main Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants