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

feat(*): allow to show one component per page #768

Closed

Conversation

stepancar
Copy link
Member

@stepancar stepancar commented Jan 9, 2018

@sapegin added simple implementation of "on component per page feature"
image

#494

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.

This is super cool!! 🦄

Minor tweaks, tests, docs ;-)

@@ -8,7 +8,7 @@ exports[`should ignore items without name 1`] = `
>
<Styled(Link)
className=""
href="#button"
href="blank#button"
Copy link
Member

Choose a reason for hiding this comment

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

This is weird.

Copy link
Member Author

@stepancar stepancar Jan 9, 2018

Choose a reason for hiding this comment

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

@sapegin have a look at master


it's because in test context location.pathname is blank. As i understand for this cases we shoul overwrite window.location for all our tests, but i think it's out of scope this PR

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK.

displayMode={displayMode}
oneComponentPerPage={styleguide.config.oneComponentPerPage}
Copy link
Member

Choose a reason for hiding this comment

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

We already pass config here, we should access it via context in non-render components and pass as a prop to render components if they need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sapegin thank you! fixed

@codecov-io
Copy link

codecov-io commented Jan 9, 2018

Codecov Report

Merging #768 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/utils/renderStyleguide.js 75% <ø> (ø) ⬆️
scripts/schemas/config.js 86.2% <ø> (ø) ⬆️
loaders/styleguide-loader.js 100% <ø> (ø) ⬆️
...omponents/ComponentsList/ComponentsListRenderer.js 100% <100%> (ø) ⬆️
src/utils/getRouteData.js 100% <100%> (ø) ⬆️
.../rsg-components/TableOfContents/TableOfContents.js 94.44% <100%> (+0.32%) ⬆️
src/rsg-components/StyleGuide/StyleGuide.js 83.33% <100%> (ø) ⬆️

@stepancar
Copy link
Member Author

@sapegin, added description in docs, added tests, fixes according your recomendations about config usage through context

@stepancar stepancar requested a review from okonet January 10, 2018 09:29
@sapegin sapegin added this to the 6.2.0 milestone Jan 10, 2018
@okonet
Copy link
Member

okonet commented Jan 10, 2018

I'm wondering if we could skip the config part and make it behave like this based on the URL? In this case we could just add additional action (i.e. icon or ⎇-click action) to the sidebar navigation items to open in the isolated mode?

The problem with the configuration IMO is that it addresses one use case whereas there are several depending on the current environment—in development I might want to have one component per page but when publishing we should be able to see all. This can't be solved with the config right now.

Thoughts?

@sapegin
Copy link
Member

sapegin commented Jan 10, 2018

That's a very good point! I guess we need both. The option is good to completely disable all components on one page mode.

I'm thinking that enabling this option by default in dev mode would be very good. Or make it not a boolean but a string: always, never, static-only, with the latter as a default value.

@okonet
Copy link
Member

okonet commented Jan 10, 2018

The option is good to completely disable all components on one page mode.

Why not URL based? My concern is that once option is here it's really hard to remove it in the future.

@sapegin
Copy link
Member

sapegin commented Jan 10, 2018

But the URL is always the same — / ;-)

@okonet
Copy link
Member

okonet commented Jan 10, 2018

What do you mean? We can come up with the url scheme to differentiate between 2 modes

@sapegin
Copy link
Member

sapegin commented Jan 10, 2018

We can’t: you open a style guide always at root.

@stepancar
Copy link
Member Author

@okonet For dev mode you can use another config

@sapegin
Copy link
Member

sapegin commented Jan 10, 2018

Sounds like too much work ;-0

@okonet
Copy link
Member

okonet commented Jan 10, 2018

Why do I need to change config in order to change the presentation of the component? This seems too much work for me as a user. But if @sapegin believes that config is the way, I’m okay with that. We can tackle that when a better router is in place, I guess.

@sapegin
Copy link
Member

sapegin commented Jan 10, 2018

I don’t see any other way: we need to know how many components to render when you open a style guide.

@okonet
Copy link
Member

okonet commented Jan 11, 2018

Not sure I'm getting it:

we need to know how many components to render when you open a style guide.

Why should the style guide behave differently in this case? We still can render all components as we do now but only display one if the URL matches? What am I missing?

Let's say we have the following URL: localhost:6060/#!/Placeholder/edit—the edit part should indicate that only one component will be rendered on this page.

@sapegin
Copy link
Member

sapegin commented Jan 11, 2018

Why should the style guide behave differently in this case? We still can render all components as we do now but only display one if the URL matches? What am I missing?

Because the whole point of this feature is not to render all component at the same page.

@stepancar
Copy link
Member Author

@okonet @sapegin maybe we can release this feature? Do you have any comments for implementation?

@sapegin
Copy link
Member

sapegin commented Jan 12, 2018

@stepancar What do you think about that?

I'm thinking that enabling this option by default in dev mode would be very good. Or make it not a boolean but a sting: always, never, static-only, with the latter as a default value.

I kind of like the idea but that may be breaking change to your current implementation.

@bluetidepro
Copy link
Collaborator

@sapegin @stepancar Just chiming in, but yessss! Love this feature. +1 for adding it so it can be one component per page always.

@sapegin sapegin removed this from the 6.2.0 milestone Jan 25, 2018
@stepancar
Copy link
Member Author

@sapegin, I think, that developers can just use another config in dev mode or use NODE_ENV.

@sapegin
Copy link
Member

sapegin commented Jan 30, 2018

Of course they can but why would they do that? For me it makes a lot of sense to have such behavior by default. I don't know how other people use Styleguidist but for I never need to see all component in dev mode.

@arielsalminen
Copy link

Would love to see this feature merged to master. Working on two different design systems that use StyleGuidist for docs and both would benefit from this feature. So basically here to show thumbs up 👍

@iamtyce
Copy link

iamtyce commented Jan 31, 2018

Would love to see individual routes per component rather than the long page showing all! Great work and good discussion here 👍

@jeffkamo
Copy link

I would also really like to see this feature merged :) This is the functionality that my team's designers would like see in our project.

@stepancar
Copy link
Member Author

@sapegin, I'll try to enable this feature in dev mode by default today.

}

renderSections() {
const { searchTerm } = this.state;
const { sections } = this.props;

const oneComponentPerPage = (this.context.config || {}).oneComponentPerPage;

Choose a reason for hiding this comment

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

It seems like this.context returns an empty object here. So oneComponentPerPage is always undefined

@@ -108,6 +108,7 @@ exports[`should filter section names 1`] = `
},
]
}
oneComponentPerPage={undefined}
Copy link
Member

Choose a reason for hiding this comment

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

Since oneComponentPerPage is boolean, can it be explicitly defined as false somewhere?

@bitionaire bitionaire mentioned this pull request Feb 21, 2018
sapegin pushed a commit that referenced this pull request Mar 21, 2018
sapegin pushed a commit that referenced this pull request Mar 22, 2018
👋 **[Support Styleguidist](https://opencollective.com/styleguidist) on Open Collective** 👋

## New features

### Page per section 🚧

This is a huge improvement for large style guides: now you can show only one component at a page instead of all components.

![](https://d3vv6lp55qjaqc.cloudfront.net/items/0W2q2K2s3n2k311O1J0y/Screen%20Recording%202018-03-22%20at%2010.06%20AM.gif)

To enable:

```js
module.exports = {
  pagePerSection: true
};
```

This is an experimental feature and we need your feedback to make it better and really useful. For example, right now there’s no isolated mode. So feel free to share your ideas [in issues](https://github.com/styleguidist/react-styleguidist/issues).

(#835 by @bitionaire and @stepancar, closes #494 and #768)

### “Fork me” ribbon

One more step to make Styleguidist usable for documenting open source projects:

![](https://d3vv6lp55qjaqc.cloudfront.net/items/1t331n2a3v0F2i290K0Z/Image%202018-03-22%20at%209.13.11%20AM.png)

New config option to enable the ribbon, define the URL and change the label:

```js
module.exports = {
  ribbon: {
    url: 'http://example.com/',
    text: 'Fork me on GitHub'
  }
};
```

And two new [theme variables](https://github.com/styleguidist/react-styleguidist/blob/master/src/styles/theme.js) to change colors: `color.ribbonBackground` and `color.ribbonText`.

(#861 by @glebez and @wkwiatek, part of #195, closes #647)

### Options to change CLI output on server start and build

Two new options, `printServerInstructions` and `printBuildInstructions`:

```js
module.exports = {
  printBuildInstructions(config) {
    console.log(
      `Style guide published to ${
        config.styleguideDir
      }. Something else interesting.`
    );
  }
};
```

(#878 by @roblevintennis, closes #876)

### Option to modify props

A new option, `updateDocs` to modify props before rendering. For example, you can load a component version number from a JSON file:

```js
module.exports = {
  updateDocs(docs) {
    if (docs.doclets.version) {
      const versionFilePath = path.resolve(
        path.dirname(file),
        docs.doclets.version
      );
      const version = require(versionFilePath).version;

      docs.doclets.version = version;
      docs.tags.version[0].description = version;
    }

    return docs;
  }
};
```

(#868 by @ryanoglesby08)

### Limited support for named exports

Styleguidist used to require default exports for components. Now you can use named exports too, though Styleguidist still supports only one component per file. I hope this change will make some users happier, see more details [in the docs](https://react-styleguidist.js.org/docs/components.html#loading-and-exposing-components).

(#825 by @marcdavi-es, closes #820 and #633)

### Allow arrays of component paths in sections

More flexibility in structuring your style guide:

```js
module.exports = {
  sections: [
    {
      name: 'Forms',
      components: [
        'lib/components/ui/Button.js',
        'lib/components/ui/Input.js'
      ]
    }
  ]
};
```

(#794 by @glebez, closes #774)

### Sort properties by `required` and `name` attributes

This change should make props tables more predictable for the users. Instead of relying on developers to sort props in a meaningful way, Styleguidist will show required props first, and sort props in each group alphabetically.

To disable sorting, use the identity function:

```javascript
module.exports = {
  sortProps: props => props
};
```

(#784 by @dotcs)

## Bug fixes

* Allow `Immutable` state in examples (#870 by @nicoffee, closes #864)
* Change template container ID to prevent clashes (#859 by @glebez, closes #753)
* Better props definitions with Flow types (#781 by @nanot1m)
@sapegin
Copy link
Member

sapegin commented Mar 30, 2018

Was finished in another PR, thanks for help anyway!

@sapegin sapegin closed this Mar 30, 2018
@sapegin sapegin deleted the feature/allow-to-show-one-component-per-page branch March 30, 2018 15:32
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.

9 participants