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

Refactoring: simplify PropsRenderer a bit #206

Merged
merged 10 commits into from
Oct 25, 2016

Conversation

rowlandekemezie
Copy link
Contributor

This PR closes #194. It replaces li/ul tags with react-group package and simplifies renderDescription. The specs were fixed to reflect this changes.

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks!

return (
<span>One of: <ul className={s.list}>{values}</ul></span>
<span> One of: <Group className={s.list} separator=", ">children={values}</Group></span>
Copy link
Member

Choose a reason for hiding this comment

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

This space wasn’t here. And this children= is something you probably forget to remove.

Copy link
Member

Choose a reason for hiding this comment

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

Now we can remove CSS of old lists: .list and .listItem.

));

return (
<span>One of type: <ul className={s.list}>{values}</ul></span>
<span>One of type: <Group className={s.list} seperator=", ">children{values}</Group></span>
Copy link
Member

Choose a reason for hiding this comment

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

And this children too.

@@ -5,7 +5,7 @@
* @returns {string}
*/
export function unquote(string) {
return string.replace(/^\'|^\"|\"$|\'$/g, '');
return string.replace(/^'|^"|"$|'$/g, '');
Copy link
Member

Choose a reason for hiding this comment

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

This looks like some merge issue. Would you mind creating a new pull request? Or you could try to recreate your branch and push --force this branch if that would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sapegin The linting has issues with unnecessary escape characters

Copy link
Member

@sapegin sapegin Oct 25, 2016

Choose a reason for hiding this comment

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

I see, it was a warning, that’s why it didn’t break the CI.

@sapegin
Copy link
Member

sapegin commented Oct 25, 2016

If you remove all now unused CSS, I’ll be happy to merge it.

@@ -56,11 +57,10 @@ function renderDescription(prop) {
let { description } = prop;
let extra = renderExtra(prop);
return (
<div>
<Group separator=" ">
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, last few things: space is default separator, no need to specify it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the walk-through

return (
<span>One of: <ul className={s.list}>{values}</ul></span>
<span>One of: <Group separator=", ">{values}</Group></span>
Copy link
Member

Choose a reason for hiding this comment

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

Here and in the next one you need an inline option, otherwise it would be rendered on a separate line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay

@@ -136,6 +133,7 @@ function renderShape(props) {
return rows;
}


Copy link
Member

Choose a reason for hiding this comment

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

And this extra line wasn’t here.

@sapegin sapegin merged commit 2c24c73 into styleguidist:master Oct 25, 2016
@sapegin
Copy link
Member

sapegin commented Oct 25, 2016

Thank you very much!

:shipit:

tizmagik added a commit that referenced this pull request Oct 27, 2016
* master:
  Update react-docgen.
  Update lint.
  Fix lint warning.
  Fix regression: colors in props table.
  Refactoring: simplify PropsRenderer a bit (#206)

# Conflicts:
#	package.json
sapegin added a commit that referenced this pull request Oct 28, 2016
Fix regression introduced in #206.
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.

Refactoring: simplify PropsRenderer a bit
2 participants