Skip to content

Conversation

@mattrosno
Copy link

Before, this plugin only worked with flat project directories, e.g.

examples
├── hello-world-example
│   ├── package.json
│   ├── index.html
│   └── index.js

Now, it works with subdirectories in projects, e.g.

examples
├── hello-world-example
│   ├── package.json
│   ├── public
│   │   └── index.html
│   ├── src
│   │   ├── index.js
│   │   └── styles.css

Did this to get a React app to work.

@mattrosno
Copy link
Author

Hi @elboman, have you had a chance to check out this fix? Please let me know if you have any questions. Thanks!

@karlhorky
Copy link
Contributor

@elboman would you be open to reviewing and merging this? It looks like a very useful feature to have.

@karlhorky
Copy link
Contributor

@mattrosno I guess your fork is available now (as per carbon-design-system/carbon-website#1074) and in order to use it in the meantime, users just need to edit their package.json as follows, is that correct?

-    "gatsby-remark-embedded-codesandbox": "^1.2.0",
+    "gatsby-remark-embedded-codesandbox": "github:mattrosno/gatsby-remark-embedded-codesandbox#release",

@elboman
Copy link
Owner

elboman commented Aug 5, 2019

Looks good in general, I think we can make getFiles return the array instead of mutating the argument. I can suggest a few changes later in the day!

@karlhorky
Copy link
Contributor

Friendly ping @elboman :)

@karlhorky
Copy link
Contributor

Hey @elboman, just circling back around to this one now.

Could you suggest those changes so this can be merged?

Copy link
Owner

@elboman elboman left a comment

Choose a reason for hiding this comment

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

Hey sorry for leaving this behind! Anyway, I left a couple of comments inline with what I was thinking about.

I noticed you mock the isDirectory in the tests, which is great for the ones we have. It would be great to add a test specifically for recursion search in folder.

Comment on lines +24 to +34
const getFiles = (baseDir, dir, files) => {
fs.readdirSync(dir).forEach(function(file) {
var subDir = path.join(dir, file);
if (fs.lstatSync(subDir).isDirectory()) {
getFiles(baseDir, subDir, files);
} else {
let filePath = path.join(dir, file);
files.push(filePath.replace(`${baseDir}/`, ''));
}
});
};
Copy link
Owner

Choose a reason for hiding this comment

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

I was thinking something like this (haven't tested it tho!)

const getFiles = (dir) => {
  const files = [];
  fs.readdirSync(dir).forEach(function(file) {
    const subDir = path.join(dir, file);
    if (fs.lstatSync(subDir).isDirectory()) {
	  files.push(...getFiles(subDir));
    } else {
      files.push(path.join(dir, file));
    } 
  });

  return files;
};

Comment on lines -49 to 76
let packageJsonFound = false;
const folderFiles = fs.readdirSync(directory);
const sandboxFiles = folderFiles
const folderFiles = [];
const sandboxFiles = [];
getFiles(directory, directory, folderFiles);

folderFiles
// we ignore the package.json file as it will
// be handled separately
.filter(file => file !== 'package.json')
.map(file => {
const fullFilePath = path.resolve(directory, file);
const content = fs.readFileSync(fullFilePath, 'utf-8');
return {
sandboxFiles.push({
name: file,
content,
};
});
});
Copy link
Owner

Choose a reason for hiding this comment

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

And then you can just do

    const sandboxFiles = getFiles(directory)
      // we ignore the package.json file as it will
      // be handled separately
      .filter(file => file !== 'package.json')
      .map(file => {
        const fullFilePath = path.resolve(directory, file);
        const content = fs.readFileSync(fullFilePath, 'utf-8');
        return {
          name: file,
          content,
        };
      });

@mAAdhaTTah
Copy link

This would be really help for a project I'm working on. @mattrosno Are you still working on this?

@mattrosno
Copy link
Author

@elboman thanks for the review! I'm not able to make these changes right now. Feel free to close if you want to do this in a separate PR, or if @mAAdhaTTah wants to see this through - all you!

@karlhorky
Copy link
Contributor

@mattrosno probably still no bandwidth to work on these changes, right?

And I guess @mAAdhaTTah, I suppose you're not able to take over here.

@elboman Maybe time to try to find someone to bring this home.

@mAAdhaTTah
Copy link

@karlhorky Yeah, I ended up just working around it by flattening my directory structure.

@karlhorky
Copy link
Contributor

Looks like this remark plugin allows for an arbitrarily deep directory structure, switching to this one:

https://github.com/kevin940726/remark-codesandbox

@karlhorky
Copy link
Contributor

@elboman for those who still want to use this plugin, would you consider merging this one?

@guyathomas
Copy link

Hey guys, forked this plugin and made it recursive if anyone else wants it https://www.npmjs.com/package/@guyathomas/gatsby-remark-embedded-codesandbox

@karlhorky
Copy link
Contributor

Thanks @guyathomas !! I guess this is the repo: https://github.com/guyathomas/gatsby-remark-embedded-codesandbox

Are you planning on providing some simple support over there? If so, maybe you could open up the Issues tab?

Or - maybe even better - @elboman could give you maintainer rights here...

@guyathomas
Copy link

Hi! Yeah, that's the repo! Yeah, a word of caution I don't plan on officially maintaining that going forward - ideally, the changes would be merged into this repo - I have this open PR. #27

if @elboman is willing to do that, I'd happily maintain / support it from here :)

@karlhorky
Copy link
Contributor

Right yeah, it would be great to get an active maintainer here!

@elboman what do you think of adding Guy as a maintainer?

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.

5 participants