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

Feature - load multiple sources #573

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

valvin1
Copy link

@valvin1 valvin1 commented Jun 9, 2019

As mentionend in #560 this feature allowes to load several sources using option sourceUrls instead of sourceUrl

      var slideshow = remark.create({
                                     sourceUrls: ['content/content1.md','content/content2.md']
                      });

@peterj
Copy link
Collaborator

peterj commented Jan 3, 2020

Hi @valvin1! Thanks for creating this PR. I'd love to merge this, could you make the following changes:

  • there's an existing loadFromUrl function in the slideshow.js and you have added a loadFromUrls function - could you refactor your function, so it can be used in the scenario when options.sourceUrl is provided. That way, we only have one function that can load from the URL, regardless if there's one or multiple of them

  • add/update the tests in slideshow_test.js to test the scenarios when multiple URLs are provided

@@ -63,6 +63,9 @@ function Slideshow (events, dom, options, callback) {
if (options.sourceUrl) {
loadFromUrl(options.sourceUrl, callback);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactor loadFromUrls and use it here -- (that way we can remove the loadFromUrl function and only have one.

Copy link
Collaborator

@peterj peterj left a comment

Choose a reason for hiding this comment

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

Check the comments - refactor loadFromUrls + review/add tests if needed to cover the new scenario.

@Jurigag
Copy link

Jurigag commented Feb 7, 2020

@valvin1 any progress here? this feature is really cool and would like to see it :)

@valvin1
Copy link
Author

valvin1 commented Feb 7, 2020

Hi @peterj @Jurigag
Peter thanks for your review unfortunately more than 6 monthes later I've forgotten this work. I'd love finishing it but not sure I'll have time to do it in the monthes to come. So if another one want to implement this feature he'll be welcome.

kbens added a commit to kbens/remark that referenced this pull request Sep 16, 2020
kbens added a commit to kbens/remark that referenced this pull request Sep 16, 2020
@kbens
Copy link

kbens commented Sep 16, 2020

Hello @peterj

I decided to try and help take @valvin1's great idea across the finish line here. I have the work in my fork here.

Would it work best to just create a new PR from there? Or should I try and pull it into his original PR branch?

Thanks

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.

4 participants