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: Added codeSamples and propsMethods settings per sections #941

Closed
wants to merge 18 commits into from

Conversation

rafaesc
Copy link
Collaborator

@rafaesc rafaesc commented Apr 17, 2018

First PR to abstract the sections setting, related with #892

return `${sections[0].components[0].name} — ${baseTitle}`;
} else if (displayMode === DisplayModes.section) {
} else if (displayMode === DisplayModes.section || displayMode === DisplayModes.example) {
return `${sections[0].name} — ${baseTitle}`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix #943

@@ -664,7 +664,7 @@ module.exports = {

> **Note:** `entry`, `externals`, `output`, `watch`, and `stats` options will be ignored. For production builds, `devtool` will also be ignored.

> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `MiniHtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
Copy link
Member

Choose a reason for hiding this comment

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

Looks like some merge issue — these changes are from another PR ;-/

@sapegin sapegin requested a review from okonet April 19, 2018 06:48
Copy link
Member

@okonet okonet left a comment

Choose a reason for hiding this comment

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

Nice work! Please merge in master plus see my comments.

const preview = <Preview code={code} evalInContext={evalInContext} />;
if (settings.noeditor) {
if (settings.noeditor || hideEditor) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move both conditions to the const?

const { displayMode } = this.context;
const hideEditor = codeSamples === CodeSamplesModes.hide;
Copy link
Member

Choose a reason for hiding this comment

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

If hideEditor is true we should not render preview since it will take time. Let put it into the condition below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I have understood that preview always should render, like this
https://github.com/styleguidist/react-styleguidist/blob/master/src/rsg-components/Playground/Playground.js#L68-L72, or how would I have to create the condition?, Thanks!

const filteredComponents = filterComponentsInSectionsByExactName(sections, targetName);
if (filteredComponents.length) {
sections = [{ components: filteredComponents }];
const filteredSections = filterComponentsInSectionsByExactName(sections, targetName);
Copy link
Member

Choose a reason for hiding this comment

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

What are these changes about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it's because in this case tries isolated so if I want that use the same section setting, its necessary that the sections have codeSamples and propsMethods if you see on the function filterComponentsInSectionsByExactName, right now returns a section.
https://github.com/styleguidist/react-styleguidist/pull/941/files#diff-5cafb86def7c61e87a06ed3f7c5b95cc

@arielsalminen
Copy link

@rafaesc Nice work! Let me know if I can help in any way here.

@@ -3,6 +3,8 @@ import ReactComponent from '../ReactComponent';
import Components from './Components';
import ComponentsRenderer from './ComponentsRenderer';

const codeSamples = 'collapse';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be CodeSamplesModes.collapse?

src/consts.js Outdated
@@ -11,3 +11,15 @@ export const DisplayModes = Object.freeze({
example: 'example',
// TODO: error (404)
});

export const CodeSamplesModes = Object.freeze({
Copy link
Member

Choose a reason for hiding this comment

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

I think ExampleModes would be better.

src/consts.js Outdated
expand: 'expand',
});

export const PropsMethodsModes = Object.freeze({
Copy link
Member

Choose a reason for hiding this comment

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

And UsageModes, we have the Usage component to render props and methods.

@@ -23,6 +23,6 @@ const sections = deepfreeze([
describe('filterComponentsInSectionsByExactName', () => {
it('should return components at any level with exact name', () => {
const result = filterComponentsInSectionsByExactName(sections, 'Image');
expect(result.map(x => x.name)).toEqual(['Image']);
expect(result[0].components.map(x => x.name)).toEqual(['Image']);
Copy link
Member

Choose a reason for hiding this comment

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

What was the reason for this change? Suspicious ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, because before returned a component list to isolate them, but how I need the codeSample and propsMethods section setting to mantain the setting, now it returns section list

@@ -5,17 +5,24 @@ import filterComponentsByExactName from './filterComponentsByExactName';
*
* @param {object} sections
* @param {string} name
* @return {Array}
* @return {Array} sections
Copy link
Member

Choose a reason for hiding this comment

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

Return shouldn't have a variable name.

*/
export default function filterComponentsInSectionsByExactName(sections, name) {
const components = [];
const output = [];
Copy link
Member

Choose a reason for hiding this comment

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

output is a very non-descriptive name. If you want to use components inside the loop, use something like filteredComponents instead of output.

@sapegin
Copy link
Member

sapegin commented May 8, 2018

Do we have any breaking changes here? Do we need any docs updates (I guess we do)?

@codecov-io
Copy link

codecov-io commented May 8, 2018

Codecov Report

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

Impacted Files Coverage Δ
scripts/schemas/config.js 87.5% <ø> (ø) ⬆️
scripts/utils/mergeWebpackConfig.js 100% <ø> (ø) ⬆️
src/rsg-components/Section/Section.js 100% <100%> (ø) ⬆️
src/utils/getRouteData.js 100% <100%> (ø) ⬆️
src/rsg-components/Playground/Playground.js 93.54% <100%> (+0.69%) ⬆️
src/utils/filterComponentsInSectionsByExactName.js 100% <100%> (ø) ⬆️
loaders/utils/getSections.js 100% <100%> (ø) ⬆️
...rc/rsg-components/ReactComponent/ReactComponent.js 85.18% <100%> (+1.85%) ⬆️
src/rsg-components/Components/Components.js 100% <100%> (ø) ⬆️
src/rsg-components/Examples/Examples.js 100% <100%> (ø) ⬆️
... and 2 more

@@ -109,6 +109,8 @@ Each section consists of (all fields are optional):
* `components` — a glob pattern string, an array of component paths or a function returning a list of components. The same rules apply as for the root `components` option.
* `sections` — array of subsections (can be nested).
* `description` — A small description of this section.
* `codeSamples` — Settings documentation initially of example code tab, uses [codeSamples](Configuration.md#codesamples).
Copy link
Member

Choose a reason for hiding this comment

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

“Initial state of the code example tab”? And similarly the next one.

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.

And I still have a question about breaking changes: will deprecated options work? Can we make a minor release?


Allows to config documentation initially of example code tab

* when is `collapse`: collapses the tab by default.
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove “when is”.

@@ -664,7 +672,7 @@ module.exports = {

> **Note:** `entry`, `externals`, `output`, `watch`, and `stats` options will be ignored. For production builds, `devtool` will also be ignored.

> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `MiniHtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
Copy link
Member

Choose a reason for hiding this comment

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

Ooops ;-/


Type: `String`, default: `collapse`

Allows to config documentation initially of example code tab
Copy link
Member

Choose a reason for hiding this comment

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

Defines the initial state of the example code tab:

@@ -55,6 +55,16 @@ Type: `String`, optional

Your application static assets folder, will be accessible as `/` in the style guide dev server.

#### `codeSamples`
Copy link
Member

Choose a reason for hiding this comment

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

They are examples, not samples — I think I’ve mentioned that before ;-)

@@ -335,6 +345,16 @@ module.exports = {
}
```

#### `propsMethods`
Copy link
Member

Choose a reason for hiding this comment

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

And I’d keep “usage” here.

@@ -43,7 +43,7 @@ module.exports = {

> **Note:** `entry`, `externals`, `output`, `watch`, and `stats` options will be ignored. For production builds, `devtool` will also be ignored.

> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `MiniHtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
> **Note:** `CommonsChunkPlugins`, `HtmlWebpackPlugin`, `UglifyJsPlugin`, `HotModuleReplacementPlugin` plugins will be ignored because Styleguidist already includes them or they may break Styleguidist.
Copy link
Member

Choose a reason for hiding this comment

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

Some unrelated changes in this file — probably merge issue.

@rafaesc rafaesc closed this Jul 2, 2018
@rafaesc rafaesc deleted the section-setting branch July 2, 2018 02:23
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