Added support for slides.com

[anuraghazra]

What:
Added support for slides.com

Why:
closes #8

Checklist:

• Documentation
• Tests
• Ready to be merged
  

[MichaelDeBoey]

@anuraghazra (#8 (comment)):

I tried to implement this, seems like it's a bit tricky to implement the shouldTransform check because slides don't have a distinct url to check for embeds

i tried this approach, is it good? do you have any idea how we can do it?

(host === 'slides.com' || host === 'www.slides.com') && !!pathname.match(/^\/.*\w\/.*\w\/?[^embed].\/?$/ig)

although it fails in this case

    'Slides url (valid) with extra pathname': {
      url: 'https://slides.com/news/math/asdasd',
      valid: false,
    },

[MichaelDeBoey]

@anuraghazra (#8 (comment)):

above check passes in all these tests

  {
    'non-Slides url': {
      url: 'https://not-a-slides-url.com',
      valid: false,
    },
    "non-Slides url ending with 'slides.com'": {
      url: 'https://this-is-not-slides.com',
      valid: false,
    },
    "non-Slides url ending with 'slides.com' and having '/embed/'": {
      url: 'https://this-is-not-codepen.io/news/math/embed',
      valid: false,
    },
    'explore page': {
      url: 'https://slides.com/explore',
      valid: false,
    },
    'random page': {
      url: 'https://slides.com/random-page',
      valid: false,
    },
    'Slides embed url': {
      url: 'https://slides.com/news/math/embed',
      valid: false,
    },
    'Slides embed url with trailing slash': {
      url: 'https://slides.com/news/math/embed/',
      valid: false,
    },
    'Slides homepage': {
      url: 'https://slides.com/',
      valid: false,
    },
    'Slides url': {
      url: 'https://slides.com/news/math',
      valid: true,
    },
    'Slides user slide': {
      url: 'https://slides.com/valentinogagliardi/django-rest#/1',
      valid: true,
    },
    'Slides url with trailing slash': {
      url: 'https://slides.com/news/math/',
      valid: true,
    },
    "Slides url having 'www' subdomain": {
      url: 'https://www.slides.com/news/math',
      valid: true,
    },
  }

[anuraghazra]

@MichaelDeBoey i tried with a different approach for checking if the url is valid or not, seems like its working, can you reviewing the code and adding more tests if necessary? (maybe needs some refactoring because i just wanted to make sure it works at first)

EDIT: also did manual testing, seems to be working fine, valid ones also works fine with extra query params

[MichaelDeBoey]

Hi @anuraghazra!

Thanks for taking the time to implement the @slides transformer.
@hakimel commented on Twitter for all the possible formats.
https://twitter.com/hakimel/status/1182724020824883202

[MichaelDeBoey]

Please also rebase on latest master so you get the newest tests added in #27. 🙂

[anuraghazra]

@MichaelDeBoey i will try to do above changes as soon as possible when i get some time.

[anuraghazra]

@MichaelDeBoey fixed the /embed#1 hash issues check it out.

[anuraghazra]

Don't know what happened there, it automatically says merged master. and my own commit msg doesn't show up. (something is wrong with my branch, or maybe i'm doing something wrong)

whatever, i pushed the changes. 😃

[MichaelDeBoey]

@anuraghazra I rebased your branch on upstream/master, so everything should be fine now 🙂

[anuraghazra]

What else should i do? 🤔

[MichaelDeBoey]

@anuraghazra I've updated the tests.
If these tests pass, I think you've covered all possible Deck urls. 🙂

[anuraghazra]

Okay! i will cover the tests. 😄

[anuraghazra]

Hey @MichaelDeBoey just wondering if its possible to create a slide named "embed"?

https://team.slides.com/teamname/embed
https://slides.com/michaeldeboey/embed

EDIT: okay it is possible https://slides.com/anuraghazra/embed/embed#/

[anuraghazra]

i changed this, i think if we provide www it should not remove www from url

FAIL  src/__tests__/Slides.js
[test]   ● get Slides iframe url › user Deck url having 'www' subdomain

[test]     Expected: "https://slides.com/kentcdodds/oss-we-want/embed"
[test]     Received: "https://www.slides.com/kentcdodds/oss-we-want/embed"

[MichaelDeBoey]

@anuraghazra I've rebased onto upstream/master again.
Still 1 tests that fails tho. 😕

Also left some comments

[MichaelDeBoey]

@anuraghazra Since we remove the www part in the other transformers, I think we should do it here too for consistency.

[anuraghazra]

@MichaelDeBoey okay updated the code, well i did not noticed that you updated some code in master branch.

[anuraghazra]

@MichaelDeBoey 🤔 why tests are not running? curious!

[anuraghazra]

@MichaelDeBoey done!

[MichaelDeBoey]

@anuraghazra One last remark and this one's good to merge 🙂

[anuraghazra]

oh wait we should also remove webkitallowfullscreen mozallowfullscreen allowfullscreen and use allow instead

[MichaelDeBoey]

@anuraghazra I'm not in favor of doing that right now tbh.
The official embed (when doing it on the Slides website) have them in their, so I'd like to keep them in there too for now.

We can still change that on a later moment.

[anuraghazra]

Okay no problem, let it be as it is.

[MichaelDeBoey]

Thanks for taking the time to implement the Slides transformer @anuraghazra! 👊

I just rebased your branch onto upstream/master, made getSlidesIFrameSrc a bit clearer and added a comment to getTrimmedPathName so people don't have to figure out what the regex is doing.

[MichaelDeBoey]

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀