Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

MTRC configuration #54

Merged
merged 8 commits into from Aug 14, 2016
Merged

MTRC configuration #54

merged 8 commits into from Aug 14, 2016

Conversation

rook91
Copy link

@rook91 rook91 commented Aug 11, 2016

Few days ago I started to use yours addon for react-storybook. It's great, but i found one thing that maybe can be improved. When I write some description for source, I also want to use my own markdown configuration in it. At the moment, the only way is to change files in markdown folder or change directly in Story.js (if there is another (better) way please tell how). In my PR I create additional prop: mtrcConf which allow to set own MTRC configuration. An example shows how to create own code syntax highlighter.

@rook91 rook91 changed the title MTRC configuration API MTRC configuration Aug 11, 2016
@roonyh
Copy link
Contributor

roonyh commented Aug 12, 2016

@rook91 Yes this could be very useful. Thanks much! I tried it locally and works perfectly!

Looks like the example you provided in the PR replaces the simple one we already had. And most of the code from the PR is concerning your example. Could you just include changes required for the new feature and remove the example story?

And a small nit :) Looks like you are using tabs for indentation in some places. Since the rest of the file uses spaces for indentation could you replace tabs in your code with spaces.

Thanks again for this!

const options = {
...defaultOptions,
..._options
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you did not mean to remove these following lines :) Removing them might break some stuff.

Copy link
Author

Choose a reason for hiding this comment

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

No. In my first attempt to solve problem I try to put mtrcConf in options object but it was inconvinient so i decided to move configuration to separete prop. Before commit I checked these lines and there are still there :)

Copy link
Contributor

Choose a reason for hiding this comment

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

So here you can see the actual changes made in this PR. There are some changes that we don't need for this feature. I think those were made unintentionally when you do a git pull. Could clean the PR up a bit by keeping only the changes you intended to make?

@rook91
Copy link
Author

rook91 commented Aug 12, 2016

According to last comment, original example was restored as well as tabs indentation are replaced by spaces. I think now will be ok (if I miss something - sorry). Some dependencies was also moved to dev section.

@thani-sh
Copy link
Contributor

"react-dom": "^0.14.7 || ^15.0.0",
"sinon": "^1.17.3"
"sinon": "^1.17.3",
"style-loader": "^0.13.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

So since you are not using any of those new modules for the feature itself you don't need to make any changes to package.json right? I think all those new modules you added are ones required by the earlier example.

@roonyh
Copy link
Contributor

roonyh commented Aug 12, 2016

@rook91 Thanks! Its much clear now. Could you checkout my comments? I think there are some unintended changes in this PR. See here for an overview of all the changes you made. Just keep the ones you need to add to make this feature work.

@rook91
Copy link
Author

rook91 commented Aug 12, 2016

These things happen when you are a newbie so forgive me for that mess. Package.json is clean now. Also index.js and Story.js should contains only that changes which are necessary.

export default {
addWithInfo(storyName, info, storyFn, _options) {
addWithInfo(storyName, info, storyFn, _options, _mtrcConf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rook91

There's one small bug in the code. No need to fix this because we can avoid it

It's not quite clear but the info parameter in this line is optional. Users can give the story function as the second argument, options as the third, etc. So the mtrcConf argument is not always the fourth argument. It could be either the third or the fourth.

As we already have an options object, we don't need to add another argument. mtrcConf can be a field in the options object. Try something like this:

const mtrcConf = { ...defaultMtrcConf };
if (_options && _options.mtrcConf) {
  Object.assign(mtrcConf, _options.mtrcConf);
}

I'll merge this as soon as it's fixed.

Copy link
Author

Choose a reason for hiding this comment

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

I check it on my example (with and without info parameter). It works without errors so I change it as you wish.
But, if someone miss info parameter there is no sense to add mtrcConf object because it's redundant. MTRC configuration passed by mtrcConf change style only in description (correct?) so if there is no info what is a point to add mtrcConf?

MTRC is used only here:

<div style={stylesheet.infoContent}>
  {MTRC(source).tree}
</div>

That is the reason I put it into separate prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @rook91
Yes, there's no point of having a mtrcConf argument when the info argument is missing but if we just consider the code without that assumption it's a bug.
I'll merge this PR now.

@thani-sh thani-sh merged commit 3746655 into storybook-eol:master Aug 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants