Added support for external links in sections #921
Conversation
docs/Components.md
Outdated
| @@ -110,6 +110,8 @@ Each section consists of (all fields are optional): | |||
| * `sections` — array of subsections (can be nested). | |||
| * `description` — A small description of this section. | |||
| * `ignore` — string/array of globs that should not be included in the section. | |||
| * `external` - an URL to be opened (in a new tab) instead of navigating to the section content | |||
There was a problem hiding this comment.
Maybe href would be more clear. external feels like a boolean flag.
There was a problem hiding this comment.
The problem with href is that ComponentsListRenderer already has a prop called href, and we can't easily use it to determine if it should be opened in new window or not because the href can be generated as a fully qualified URL for sections with absolute = true.
Should I try to figure this out or use externalHref or some other field?
There was a problem hiding this comment.
I think that would make this part even more clear:
href: item.href || getUrl({ ...
external: !!item.href
There was a problem hiding this comment.
You don't have to use the same API for users and developers ;-)
docs/Components.md
Outdated
| }, | ||
| { | ||
| name: 'Live Demo', | ||
| skip: true, |
There was a problem hiding this comment.
Shouldn’t skip be implied for external links?
There was a problem hiding this comment.
Ok, so basically remove the "skip" and just have the renderer skip external sections ?
There was a problem hiding this comment.
I think that would make the API easier to use.
src/utils/getUrl.js
Outdated
| { origin, pathname } = window.location | ||
| ) { | ||
| let url = pathname; | ||
|
|
||
| if (external) { |
There was a problem hiding this comment.
You could move it before let url….
src/rsg-components/Link/Link.spec.js
Outdated
| @@ -23,3 +23,13 @@ it('should compose passed class names', () => { | |||
|
|
|||
| expect(actual.find('a').prop('className')).toBe('baseLinkClass customClass'); | |||
| }); | |||
|
|
|||
| it('should open external links in new window', () => { | |||
There was a problem hiding this comment.
We’re not really testing that it opens a link in a new window ;-) “Should render external links with _blank target” would be more clear.
Codecov Report
|
|
Hello @sapegin - I've update as per your recommendation |
|
I see that the coverage is lowered - please let me know if I should provide additional tests. |
| anchor: !useIsolatedLinks, | ||
| isolated: useIsolatedLinks, | ||
| }), | ||
| target: item.href ? '_blank' : undefined, |
There was a problem hiding this comment.
external would be better, _blank is an implementation detail, we shouldn't know about that here.
There was a problem hiding this comment.
Do you mean to use external string instead of _blank? If so, I'm not sure about that one. _blank is guaranteed to open a new tab every time, if we use and external multiple links will open in the same window.
There was a problem hiding this comment.
I mean making an API that isn't tied to implementation: external: true and then:
target={item.external ? '_blank' : undefined}
| @@ -6,7 +6,9 @@ import SectionsRenderer from 'rsg-components/Sections/SectionsRenderer'; | |||
| export default function Sections({ sections, depth }) { | |||
| return ( | |||
| <SectionsRenderer> | |||
| {sections.map((section, idx) => <Section key={idx} section={section} depth={depth} />)} | |||
| {sections | |||
| .filter(section => !section.href) | |||
There was a problem hiding this comment.
What's the use case for sections without href?
There was a problem hiding this comment.
Currently any section added to the configuration will result in a section space being generated in the main content - this would also include the href sections which should open in new window.
This will filter out those sections from being rendered in the main content area.
| {sections.map((section, idx) => <Section key={idx} section={section} depth={depth} />)} | ||
| {sections | ||
| .filter(section => !section.href) | ||
| .map((section, idx) => <Section key={idx} section={section} depth={depth} />)} |
There was a problem hiding this comment.
Do you think we can improve key={idx} now? Is there anything unique for each section that we can use as a key instead of an index?
There was a problem hiding this comment.
Dont thinks so, unless we assume that all sections have unique hrefs, which they might now have, so I guess we are stuck with the index for now.
There was a problem hiding this comment.
Then title + href should be unique enough, I'd really like to avoid indexes.
There was a problem hiding this comment.
Turns out that some sections do not have slug, but are rendered with an array of components - this is for example visible in sections example, WrappedButton link.
…re you dont get section slug
docs/Cookbook.md
Outdated
| @@ -119,7 +119,7 @@ export default icons | |||
|
|
|||
| ````jsx | |||
| // IconGallery.md | |||
| ```jsx noeditor | |||
| ;```jsx noeditor | |||
There was a problem hiding this comment.
Not sure what's going on here; this looks to be changed by a pre-commit hook, and probably should?
There was a problem hiding this comment.
It looks like the file was edited in the browser so the precommit hook wasn't run ;-/ Could you please change jsx to markdown for the whole block? It'll stop Prettier from adding ;.
There was a problem hiding this comment.
I assume jsx static won't work here? Updating.
There was a problem hiding this comment.
It's a bit confusing: here's an example of what you should write in Markdown, so the outer tag should be markdown instead of jsx. jsx noeditor part is fine — it's part of an example.
There was a problem hiding this comment.
Yes, I was thinking ````jsx static would work in order to keep highlights. Should be like you asked now; I think the newline was added by precommit hook again.
👋 **[Support Styleguidist](https://opencollective.com/styleguidist) on Open Collective** 👋 ## New features * Added support for external links in sections ([#921](#921) by @BTMPL) Two flags added to `Sections` config: * `href` - An URL to navigate to instead of navigating to the section content * `external` - If set, the link will open in a new window
|
🎉 This PR is included in version 7.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
👋 **[Support Styleguidist](https://opencollective.com/styleguidist) on Open Collective** 👋 ## New features * Added support for external links in sections ([#921](styleguidist/react-styleguidist#921) by @BTMPL) Two flags added to `Sections` config: * `href` - An URL to navigate to instead of navigating to the section content * `external` - If set, the link will open in a new window
This PR adds support for external links in the sidebar (for example to link to external documentation or more extensive demos).
The two added flags are:
section.external- for the linksection.skip- skips rendering of the section in the main content (for both external links and logical grouping in the sidebar only)