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

Page per section #835

Merged
merged 28 commits into from
Mar 21, 2018
Merged

Conversation

bitionaire
Copy link
Contributor

Added routing variant that will show sections & components in isolated mode (when pagePerSection is set to true in the config part), starting with the first section or component for the index page.

There are some minor changes and tweaks in comparison to PR #768. Instead of showing the first component per default, I set the first section or component as the default. I assume most of the larger styleguides start with some kind of Intro or Getting started section anyways.

This PR should address #494.

What do you think about it?

@codecov-io
Copy link

codecov-io commented Feb 21, 2018

Codecov Report

Merging #835 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted Files Coverage Δ
scripts/schemas/config.js 86.2% <ø> (ø) ⬆️
loaders/styleguide-loader.js 100% <ø> (ø) ⬆️
...rc/rsg-components/ComponentsList/ComponentsList.js 100% <100%> (ø)
src/utils/getRouteData.js 100% <100%> (ø) ⬆️
src/utils/renderStyleguide.js 76.92% <100%> (+1.92%) ⬆️
...omponents/ComponentsList/ComponentsListRenderer.js 100% <100%> (ø) ⬆️
src/rsg-components/slots/index.js 100% <100%> (ø) ⬆️
.../rsg-components/TableOfContents/TableOfContents.js 94.11% <100%> (ø) ⬆️
src/rsg-components/StyleGuide/StyleGuide.js 86.66% <100%> (+3.33%) ⬆️

@sapegin
Copy link
Member

sapegin commented Feb 21, 2018

Awesome, thanks for picking it up! I haven't looked into the code yet, let's finalize the API first.

Instead of showing the first component per default, I set the first section or component as the default. I assume most of the larger styleguides start with some kind of Intro or Getting started section anyways.

I thinks that's a great idea!

What do you think about making the option a string instead of a boolean with values always, never, static-only, with the latter as a default value? static-only will show one component in dev mode and all in the static build, I believe that would be most convenient for the majority of users.

@SaraVieira
Copy link
Collaborator

Hi @bitionaire!

This is awesome! This is just what I needed to solve a major problem in our styleguide! Anxiously awaiting for this to get merged!

@bitionaire
Copy link
Contributor Author

What do you think about making the option a string instead of a boolean with values always, never, static-only, with the latter as a default value? static-only will show one component in dev mode and all in the static build, I believe that would be most convenient for the majority of users.

Sure, maybe there are even more modes we didn't think of yet. Good point! 👍
I also noticed that the page doesn't scroll to top after #!/ route changes. I'll fix that, too.

@bitionaire
Copy link
Contributor Author

I just added a few commits, and pagePerSection may be defined as a function. static-only may look like pagePerSection: () => (process.env.NODE_ENV !== 'production').

I added window.scrollTo(0,0) in componentDidUpdate of StyleGuide.js. The location of this code seems really hacky, and it does not work for all use-cases (e.g. scrolling down and navigating to the same section won't trigger the execution), but I'm not sure where to put it. I'll think about a better solution. If you have any ideas, I'm open to any suggestions :)

@tizmagik
Copy link
Collaborator

Looking forward to this! 🙏

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 again for the PR! A few small remarks.

>
<Styled(Link)
className=""
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.

Is this correct value? Looks like a non-isolated link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I missed that in my refactoring

@@ -44,6 +47,15 @@ export default class StyleGuide extends Component {
};
}

componentDidUpdate(prevProps) {
// scroll to top of styleguide container when sections changed
Copy link
Member

Choose a reason for hiding this comment

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

Probably this will be a better place: https://github.com/styleguidist/react-styleguidist/blob/master/src/utils/renderStyleguide.js

Though you may need to cache current route.

Copy link
Contributor Author

@bitionaire bitionaire Feb 23, 2018

Choose a reason for hiding this comment

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

I thought about that. It's way more efficient to watch on location changes, yet it would mean to replace the functional component something like an "App" component or to merge the logic with the StyleGuide.js. What do you think is the better way?

hasSidebar={config.showSidebar && displayMode === DisplayModes.all}
toc={<TableOfContents sections={allSections} useIsolatedLinks={pagePerSection} />}
hasSidebar={
(pagePerSection && displayMode !== DisplayModes.example) ||
Copy link
Member

Choose a reason for hiding this comment

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

I guess it's time to move this condition to a function with a good comment ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right

@@ -27,7 +28,9 @@ export default function renderStyleguide(
// all components accessible to all examples
globalizeComponents(allSections);

const { sections, displayMode } = getRouteData(allSections, loc.hash);
const { pagePerSection } = styleguide.config;
const renderPagePerSection = isFunction(pagePerSection) ? pagePerSection() : pagePerSection;
Copy link
Member

Choose a reason for hiding this comment

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

We should convert function to a boolean in config schema and later use boolean values, would be simpler.

But since we don't pass any dynamic data to this function, we probably don't need it. This would also work:

module.exports = {
  pagePerSection: process.env.NODE_ENV !== 'production'
}

Copy link
Contributor Author

@bitionaire bitionaire Feb 23, 2018

Choose a reason for hiding this comment

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

Sure. Just reverted the changes. So there's no need for an enum like always, never or static-only, right?

Copy link
Member

@sapegin sapegin Feb 23, 2018

Choose a reason for hiding this comment

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

I think I could just add this as a default value in the next major release.

src/index.js Outdated
let codeRevision = 0;

const render = () => {
// eslint-disable-next-line import/no-unresolved
const styleguide = require('!!../loaders/styleguide-loader!./index.js');
ReactDOM.render(renderStyleguide(styleguide, codeRevision), document.getElementById('app'));
ReactDOM.render(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it was necessary: it should rerender everything on hash change, so you can scroll in renderStyleguide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to introduce any kind of global variable; either the hash of the last location or a boolean that will indicate if an event listener for hashchange was already registered.
I think it'll be easier to handle the component (or event handler) lifecycle in the <App />. But it's your project so you decide. Should I revert the changes and add an event listener for the first time renderStyleguide will be called?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please. I think it makes this part much more complex then it was.

What kind of an event handler you want to add? There's a hashchange handler already that rerenders everything.

@bitionaire
Copy link
Contributor Author

Allright, please have look 👍

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 for the changes! Just a few small things left 🦄

src/index.js Outdated
let codeRevision = 0;

const render = () => {
// eslint-disable-next-line import/no-unresolved
const styleguide = require('!!../loaders/styleguide-loader!./index.js');
ReactDOM.render(renderStyleguide(styleguide, codeRevision), document.getElementById('app'));
ReactDOM.render(renderStyleguide(styleguide, status), document.getElementById('app'));
if (get(window, 'location.hash', '').startsWith('#!/')) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. This needs a comment.

  2. Would it be triggered on code updates too? Probably better to have a separate hashchange just for this.

  3. I guess status instead of codeRevision wasn't intended.

src/index.js Outdated
@@ -4,14 +4,19 @@ import './polyfills';
import './styles';
import ReactDOM from 'react-dom';
import renderStyleguide from './utils/renderStyleguide';
import { get } from 'lodash';
Copy link
Member

Choose a reason for hiding this comment

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

Please don't import whole Lodash on frontend. But I think you don't really need it here, are there any cases where location.hash won't be defined?

@@ -1 +1 @@
export { default } from 'rsg-components/ComponentsList/ComponentsListRenderer';
export { default } from './ComponentsList.js';
Copy link
Member

Choose a reason for hiding this comment

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

All component imports should be like rsg-components/ComponentsList/ComponentsListRenderer, see more info here: https://react-styleguidist.js.org/docs/development.html

}

return { sections, displayMode };
}

function getFirstSectionOrComponent(sections) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this function before the default export and use an arrow function? That's the convention we use for such smallish functions.

@bitionaire
Copy link
Contributor Author

I think we're getting close 🍻

src/index.js Outdated
let codeRevision = 0;

/** Scrolls to origin when current window location hash points to an isolated view. */
const scrollToOrigin = () => {
if (window.location.hash.startsWith('#!/')) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid startsWith won't work in IE11, everything else looks fine for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! I replaced it with indexOf === 0

@sapegin sapegin added this to the 6.3.0 milestone Mar 4, 2018
@sapegin
Copy link
Member

sapegin commented Mar 4, 2018

Awesome, thanks! I'll try to make a release soon 🦄

@bitionaire
Copy link
Contributor Author

Hey @sapegin,

I've added one change, can you have a look at that?
It's hard to define a correct behaviour for the IsolateButton if pagePerSection=true. Therefore I excluded the button from any toolbars for the slots object, based on the styleguide configuration.

@sapegin sapegin merged commit 6055c2b into styleguidist:master 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 22, 2018

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

6 participants