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

Youtube Short URL / List URL support #9

Closed
wants to merge 3 commits into from

Conversation

parsher
Copy link

@parsher parsher commented Dec 13, 2019

Hello. Thank you for creating this component.
I added some features. Would you merge this?
If you don't like my code styles, please modify anything that you want.

Added features:

  • Youtube Short URL support / List URL support

History:

  • URL validation added
  • URL regex added
  • generateURL modified
  • data id added
  • data urlQuery added
  • setUrlInformation created

url validation added,  url regex added, generateURL modified, data id added, data urlQuery added, setUrlInformation created
@andrewvasilchuk
Copy link
Owner

andrewvasilchuk commented Dec 13, 2019

@parsher, Thank you for your PR. Maybe we should be more declarative? I do not think it's good idea to introduce watch and method instead of computed. To support short URL we should change the regExp a bit. I would like to merge your PR, with a new regExp :)

Or should I do it itself?

@andrewvasilchuk andrewvasilchuk added the enhancement New feature or request label Dec 13, 2019
Copy link
Owner

@andrewvasilchuk andrewvasilchuk left a comment

Choose a reason for hiding this comment

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

Enhance regExp to support short URL.

@sbagdaulet
Copy link

sbagdaulet commented Dec 13, 2019

/(?:(?:v=|(?:v%3D))|(?:\/(?:v|e)\/)|(?:(?:(?:youtu\.be)|(?:embed))\/))([^&\n#?%]+)/

Try this.

Copy link
Author

@parsher parsher left a comment

Choose a reason for hiding this comment

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

I changed some regexes, and move data properties to computed. How about this?

@andrewvasilchuk
Copy link
Owner

andrewvasilchuk commented Dec 14, 2019

Why do we need the urlQuery computed property? There is already a query prop you can pass along. Do you want to extract the query part of the passed url prop, and append it to the query prop? Also I think we should use regExp from this SO answer. I also think that we should drop validator for the url prop, because we can't handle all the cases.

@andrewvasilchuk
Copy link
Owner

The list of all URL formats of YouTube.

@andrewvasilchuk
Copy link
Owner

andrewvasilchuk commented Dec 14, 2019

ASAP I have free time I am going to support 90% of the available YouTube URL's. Also I'am going to introduce a videoId prop.

@parsher
Copy link
Author

parsher commented Dec 15, 2019

@andrewvasilchuk Yes, we don't need the urlQuery, if we supply the query prop manually. I just want that this component can handle all the possible query parameters automatically, even if I didn't pass that ones. If you have a time for handling this, I think that I should close this PR. Thanks for checking this!

@parsher parsher closed this Dec 18, 2019
@andrewvasilchuk andrewvasilchuk self-assigned this Jan 3, 2020
@andrewvasilchuk
Copy link
Owner

Closed via c22b455. It's only supported in 1.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants