POC: Extract Markdown links and headers as components. #613 #619

ryanoglesby08 · 2017-10-01T18:49:20Z

... so that they can be overridden in styleguide.config.js.

…n be overridden in styleguide.config.js. styleguidist#613

ryanoglesby08 · 2017-10-01T18:49:59Z

Re: Issue #613

ryanoglesby08 · 2017-10-01T18:57:38Z

This is a WIP and more of a proof of concept right now. I wanted to go ahead and get a PR out there so that I can get some feedback before continuing further down this path.

My strategy is to break up the large styles object in "rsg-components/Markdown" and extract components for each element. I've done that for a and h1-h6. This will allow each Markdown element/component to be overridden using the styleguideComponents config option. When overriding the Markdown components, you will export a markdown-to-jsx config object containing the new component and any props.

I've overridden links and headers in this manner in the customised example.

This seems to be consistent with the patterns already established, flexible, allow custom theming and styles to still work, and does not introduce much extra complexity.

Let me know what you think!

sapegin · 2017-10-02T07:35:07Z

Hey @ryanoglesby08, thank you very much for your pull request! I’m just back from React Alicante (it was amazing!) and will have a look soon ;-)

sapegin

I like the general idea of breaking the UI into small peaces, but I think the implementation is more complex than it could be:

We already have a Link component so you can reuse it directly. I’d extract Header component the same way so it could be reused not only in Markdown. And I’m not sure we need to store these configs in separate files.

ryanoglesby08 · 2017-10-03T00:24:30Z

Re: I’d extract Header component the same way so it could be reused not only in Markdown.

Sure. I can do that eventually.

Re: And I’m not sure we need to store these configs in separate files.

By "configs" I assume you mean the mappings of component + props to the corresponding tag name that gets translated by markdown-to-jsx.

I think storing the configs in separate files enables modularity and creates a more flexible solution. By storing the configs in separate files, it allows the react component library to define its own mappings per component without having to override a giant Markdown.js file.

Overriding any Markdown element with a new component requires 2 things: 1) the new component and 2) a new mapping of the Markdown element to the component and props. If I did not keep the configs separate, it means more files need to change to override a single Markdown element.

Do you have other ideas for how I might approach this?

sapegin · 2017-10-03T15:15:01Z

I think storing the configs in separate files enables modularity and creates a more flexible solution. By storing the configs in separate files, it allows the react component library to define its own mappings per component without having to override a giant Markdown.js file.

Do you see many use cases when people would want to define separate components for each heading level? Do you think it’s important to differentiate components (link, header, etc.) that are used in Markdown and the same components that are used in Styleguidist UI? I think using normal component (when you export a real component) increases modularity and much less surprising.

Overriding any Markdown element with a new component requires 2 things: 1) the new component and 2) a new mapping of the Markdown element to the component and props. If I did not keep the configs separate, it means more files need to change to override a single Markdown element.

In both cases you would need to override one file: file that exports component or file that exports mapping and hides component inside.

… override. styleguidist#613

ryanoglesby08 · 2017-10-13T14:43:59Z

Hi @sapegin. I've finally been able to work a bit more on this. I've changed the approach a little bit based on your feedback. This feels like a good approach to me. Here is the structure I've come out with so far. Let me know of your feedback.

rsg-components
|
| ->Heading/HeadingRenderer.js: a h1 - h6 component used throughout styleguidist, including Markdown files (Override me to replace all headings)
|
|
|-> Markdown/Heading.js: An adapter on top of HeadingRenderer that adds bottom margin. This is useful for the prose in Markdown, but not wanted on all headings throughout the style guide, such as the SectionHeadings. (Override me to replace how much spacing is added around headings in Markdown)
|
|-> Markdown/Markdown.js: Contains the mapping of all components to elements in Markdown

examples/customised

src/components
| -> Header.js: A custom h1 - h6 component being showcased

styleguide/components
| -> Heading.js: An adapter that adapts the interface of "rsg-components/Heading/HeadingRenderer" to the Header that is being showcased. Namely adapting the depth prop to the level prop, which have different types.
| -> Link.js: A Link component to be used in the styleguide with custom link styling. This is just an example of how to incorporate plain styles and another overridden component.

styleguide.config.js
—> alias HeadingRenderer with "styleguide/components/Heading"
—> alias Link with "styleguide/components/Link"

sapegin

I like it much more, thanks!

sapegin · 2017-10-16T08:38:21Z

src/rsg-components/Heading/HeadingRenderer.js

+}
+
+export const styles = ({ color, fontSize, fontFamily }) => ({
+	heading: {


I’d compose using cx() function — would make code simpler and shorter.

sapegin · 2017-10-16T08:39:42Z

src/rsg-components/Markdown/Heading.js

+
+import Styled from 'rsg-components/Styled';
+
+import HeadingRenderer from 'rsg-components/Heading/HeadingRenderer';


Should be import HeadingRenderer from 'rsg-components/Heading' — *Render components are implementation detail, we should’t leak to the caller code. See for details: https://react-styleguidist.js.org/docs/development.html

sapegin · 2017-10-16T08:40:10Z

src/rsg-components/Heading/HeadingRenderer.js

+import cx from 'classnames';
+import Styled from 'rsg-components/Styled';
+
+function HeadingRenderer({ classes, depth, id, deprecated, children }) {


level sounds better than depth for me.

Sure can use level. I used depth because that prop name was already in place from the SectionHeadingRenderer, which is where this component was extracted from.

I see ;-) This line is funny:

const headingLevel = Math.min(6, depth);

Let’s make it level everywhere.

sapegin · 2017-10-16T08:47:44Z

src/rsg-components/Markdown/Heading.js

+
+function MarkdownHeading({ classes, depth, children }) {
+	return (
+		<div className={classes.spacing}>


Maybe Heading component should accept className? And we could get rid of the div here.

sapegin · 2017-10-16T08:50:03Z

src/rsg-components/SectionHeading/SectionHeadingRenderer.js

-			<div className={classes.toolbar}>{toolbar}</div>
-		</Tag>
+		<HeadingRenderer id={id} depth={depth} deprecated={deprecated}>
+			<div className={classes.root}>


And get rid or this div as well ;-)

ryanoglesby08 · 2017-10-17T22:45:40Z

ryanoglesby08 · 2017-10-17T22:49:08Z

Also, I noticed that the Markdown.js file has styles for "input" elements. Is this necessary? There is no Markdown syntax for inputs, and I'm not sure the purpose of trying to embed inputs in Markdown documentation.

https://github.com/styleguidist/react-styleguidist/blob/master/src/rsg-components/Markdown/Markdown.js#L121-L125

sapegin · 2017-10-18T06:53:47Z

Maybe better to split into two (or more PRs) if that’s convenient for you.

Also, I noticed that the Markdown.js file has styles for "input" elements. Is this necessary?

They are for todos like in your previous comment ;-)

ryanoglesby08 · 2017-10-20T18:41:46Z

Cool, will break it up into multiple PRs. Will leave this one here to keep the todo list if you don't mind.

sapegin · 2017-10-20T18:45:17Z

I’d finish this one because it looks very close to done and then continue when you have time.

quantizor · 2017-12-03T07:46:04Z

@ryanoglesby08 actually GFM task lists will render inputs: https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments

GFM is supported in markdown-to-jsx

ryanoglesby08 · 2018-03-07T19:17:49Z

All elements within Markdown now have associated components, making them easy to override consumers. Closing this PR. Thanks for all the suggestions and help @sapegin

POC: Extract Markdown links and headers as components so that they ca…

ed46ab7

…n be overridden in styleguide.config.js. styleguidist#613

sapegin requested changes Oct 2, 2017

View reviewed changes

Extract a HeadingRenderer for use in all headings, and show sample of…

59df4df

… override. styleguidist#613

sapegin requested changes Oct 16, 2017

View reviewed changes

ryanoglesby08 mentioned this pull request Oct 24, 2017

Introduce heading components #651

Merged

ryanoglesby08 mentioned this pull request Dec 3, 2017

Custom React components for Markdown elements #613

Closed

ryanoglesby08 mentioned this pull request Dec 22, 2017

Feat: Introduce List components for Markdown. #755

Merged

ryanoglesby08 closed this Mar 7, 2018

ryanoglesby08 deleted the custom-react-components-for-markdown-components branch March 7, 2018 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: Extract Markdown links and headers as components. #613 #619

POC: Extract Markdown links and headers as components. #613 #619

ryanoglesby08 commented Oct 1, 2017

ryanoglesby08 commented Oct 1, 2017

ryanoglesby08 commented Oct 1, 2017

sapegin commented Oct 2, 2017

sapegin left a comment

ryanoglesby08 commented Oct 3, 2017

sapegin commented Oct 3, 2017

ryanoglesby08 commented Oct 13, 2017

sapegin left a comment

sapegin Oct 16, 2017

sapegin Oct 16, 2017

sapegin Oct 16, 2017

ryanoglesby08 Oct 17, 2017

sapegin Oct 18, 2017

sapegin Oct 16, 2017

sapegin Oct 16, 2017

ryanoglesby08 commented Oct 17, 2017 •

edited

ryanoglesby08 commented Oct 17, 2017

sapegin commented Oct 18, 2017

ryanoglesby08 commented Oct 20, 2017

sapegin commented Oct 20, 2017

quantizor commented Dec 3, 2017

ryanoglesby08 commented Mar 7, 2018


		import Styled from 'rsg-components/Styled';

		import HeadingRenderer from 'rsg-components/Heading/HeadingRenderer';

POC: Extract Markdown links and headers as components. #613 #619

POC: Extract Markdown links and headers as components. #613 #619

Conversation

ryanoglesby08 commented Oct 1, 2017

ryanoglesby08 commented Oct 1, 2017

ryanoglesby08 commented Oct 1, 2017

sapegin commented Oct 2, 2017

sapegin left a comment

Choose a reason for hiding this comment

ryanoglesby08 commented Oct 3, 2017

sapegin commented Oct 3, 2017

ryanoglesby08 commented Oct 13, 2017

sapegin left a comment

Choose a reason for hiding this comment

sapegin Oct 16, 2017

Choose a reason for hiding this comment

sapegin Oct 16, 2017

Choose a reason for hiding this comment

sapegin Oct 16, 2017

Choose a reason for hiding this comment

ryanoglesby08 Oct 17, 2017

Choose a reason for hiding this comment

sapegin Oct 18, 2017

Choose a reason for hiding this comment

sapegin Oct 16, 2017

Choose a reason for hiding this comment

sapegin Oct 16, 2017

Choose a reason for hiding this comment

ryanoglesby08 commented Oct 17, 2017 • edited

To do:

ryanoglesby08 commented Oct 17, 2017

sapegin commented Oct 18, 2017

ryanoglesby08 commented Oct 20, 2017

sapegin commented Oct 20, 2017

quantizor commented Dec 3, 2017

ryanoglesby08 commented Mar 7, 2018

ryanoglesby08 commented Oct 17, 2017 •

edited