-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Section isolated mode #394
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
Changes from all commits
d24d937
de6dd2d
c0a9856
f2ddd58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,24 +2,52 @@ import React from 'react'; | |
import PropTypes from 'prop-types'; | ||
import Styled from 'rsg-components/Styled'; | ||
import Heading from 'rsg-components/Heading'; | ||
import Link from 'rsg-components/Link'; | ||
|
||
const styles = ({ space, fontFamily, fontSize }) => ({ | ||
root: { | ||
marginBottom: space[4], | ||
'&:hover $isolatedLink': { | ||
isolate: false, | ||
opacity: 1, | ||
}, | ||
}, | ||
heading: { | ||
margin: [[0, 0, space[2]]], | ||
fontFamily: fontFamily.base, | ||
fontSize: fontSize.h1, | ||
}, | ||
isolatedLink: { | ||
position: 'absolute', | ||
top: 0, | ||
right: 0, | ||
fontFamily: fontFamily.base, | ||
fontSize: fontSize.base, | ||
opacity: 0, | ||
transition: 'opacity ease-in-out .15s .2s', | ||
}, | ||
titleWrapper: { | ||
position: 'relative', | ||
}, | ||
}); | ||
|
||
export function SectionRenderer({ classes, name, slug, content, components, sections }) { | ||
export function SectionRenderer({ classes, name, slug, content, components, sections, isolatedSection }) { | ||
return ( | ||
<section className={classes.root}> | ||
{name && ( | ||
<Heading level={1} slug={slug} className={classes.heading}>{name}</Heading> | ||
)} | ||
<div className={classes.titleWrapper}> | ||
{name && ( | ||
<Heading level={1} slug={slug} className={classes.heading}>{name}</Heading> | ||
)} | ||
<div className={classes.isolatedLink}> | ||
{name && ( | ||
isolatedSection ? ( | ||
<Link href="/">⇽ Back</Link> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And a test for back link. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to understand how to add tests for links, but I couldn't figure it out. Do you mind adding it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That’s fine, we’re working on a new UI for these links anyway ;-) |
||
) : ( | ||
<Link href={'#!/' + name}>Open isolated ⇢</Link> | ||
) | ||
)} | ||
</div> | ||
</div> | ||
{content} | ||
{components} | ||
{sections} | ||
|
@@ -34,6 +62,7 @@ SectionRenderer.propTypes = { | |
content: PropTypes.node, | ||
components: PropTypes.node, | ||
sections: PropTypes.node, | ||
isolatedSection: PropTypes.bool, | ||
}; | ||
|
||
export default Styled(styles)(SectionRenderer); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,30 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`render should not render title if name is not set 1`] = `<section />`; | ||
exports[`render should not render title if name is not set 1`] = ` | ||
<section> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These empty divs look strange but they most likely will go away after UI 2.0 ;-) |
||
<div> | ||
<div /> | ||
</div> | ||
</section> | ||
`; | ||
|
||
exports[`render should render component 1`] = ` | ||
<section> | ||
<_class | ||
level={1} | ||
slug="foo" | ||
> | ||
Foo | ||
</_class> | ||
<div> | ||
<_class | ||
level={1} | ||
slug="foo" | ||
> | ||
Foo | ||
</_class> | ||
<div> | ||
<_class | ||
href="#!/Foo" | ||
> | ||
Open isolated ⇢ | ||
</_class> | ||
</div> | ||
</div> | ||
<Examples | ||
examples={ | ||
Array [ | ||
|
@@ -36,24 +51,35 @@ exports[`render should render component 1`] = ` | |
|
||
exports[`render should render title if name is set 1`] = ` | ||
<section> | ||
<_class | ||
level={1} | ||
slug="test" | ||
> | ||
test | ||
</_class> | ||
<div> | ||
<_class | ||
level={1} | ||
slug="test" | ||
> | ||
test | ||
</_class> | ||
<div> | ||
<_class | ||
href="#!/test" | ||
> | ||
Open isolated ⇢ | ||
</_class> | ||
</div> | ||
</div> | ||
</section> | ||
`; | ||
|
||
exports[`should not render components list if not defined 1`] = ` | ||
<_class | ||
isolatedSection={false} | ||
name="No components" | ||
slug="no-components" | ||
/> | ||
`; | ||
|
||
exports[`should not render sections if not defined 1`] = ` | ||
<_class | ||
isolatedSection={false} | ||
name="No sections" | ||
slug="no-sections" | ||
/> | ||
|
@@ -83,6 +109,7 @@ exports[`should render component renderer 1`] = ` | |
} | ||
/> | ||
} | ||
isolatedSection={false} | ||
name="Foo" | ||
sections={ | ||
<Sections | ||
|
@@ -100,13 +127,15 @@ exports[`should render components list 1`] = ` | |
components={Array []} | ||
/> | ||
} | ||
isolatedSection={false} | ||
name="Components" | ||
slug="components" | ||
/> | ||
`; | ||
|
||
exports[`should render sections if defined 1`] = ` | ||
<_class | ||
isolatedSection={false} | ||
name="Nested sections" | ||
sections={ | ||
<Sections | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,21 +149,31 @@ export function filterComponentsInSectionsByExactName(sections, name) { | |
} | ||
|
||
/** | ||
* Returns an object containing component name and, optionally, an example index | ||
* Filters the sections to find the one with the matching name | ||
* @param {Array} sections The styleguide sections | ||
* @param {string} name The name to match | ||
* @return {object} The section found | ||
*/ | ||
export function filterSections(sections, name) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test for this function too? :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added tests for this function. |
||
return sections.find(section => section.name === name); | ||
} | ||
|
||
/** | ||
* Returns an object containing component/section name and, optionally, an example index | ||
* from hash part or page URL: | ||
* http://localhost:6060/#!/Button → { targetComponentName: 'Button' } | ||
* http://localhost:6060/#!/Button/1 → { targetComponentName: 'Button', targetComponentIndex: 1 } | ||
* http://localhost:6060/#!/Button → { targetName: 'Button' } | ||
* http://localhost:6060/#!/Button/1 → { targetName: 'Button', targetIndex: 1 } | ||
* | ||
* @param {string} [hash] | ||
* @returns {object} | ||
*/ | ||
export function getComponentNameFromHash(hash = window.location.hash) { | ||
export function getInfoFromHash(hash = window.location.hash) { | ||
if (hash.substr(0, 3) === '#!/') { | ||
const tokens = hash.substr(3).split('/'); | ||
const index = parseInt(tokens[1], 10); | ||
return { | ||
targetComponentName: tokens[0], | ||
targetComponentIndex: isNaN(index) ? null : index, | ||
targetName: tokens[0], | ||
targetIndex: isNaN(index) ? null : index, | ||
}; | ||
} | ||
return {}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it’s time to extract it to a separate component. We already have this pattern in two places I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an IsolatedLink component that contains the markup and the CSS for it. I believe that the CSS rule that says "on hover of this element, reveal the IsolatedLink" should be inside the component (the component would just need the className for the element that will reveal the component once that element is hovered).
Do you agree with this approach?
Trying to implement this, I run into a problem with how to use this class inside the styles object. Following is the IsolateLinkRender where hoverClass is the class I'm getting from the parent component, and trying to use it in the styles object. Obviously the following doesn't work, since I don't know how to pass the hoverLink from the component to the styles object. Any help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this I agree.
I think you need to put both of them into this component. Sou it’d be not
IsolatedLink
but more likeIsolatedContainer
:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sapegin I created the IsolatedContainer component. Since the CSS rule for "onHover" is inside the component, that means that on heading hover show the "Open Isolated" link (it used to be onHover of the whole component info area show the link). That seems fine by me, but theres a problem with the Isolated example which has no title. It used to be on hover of the code example area but now the only "hover" area is the link itself which is hidden.
Any ideas? Do you want me to update the PR to see the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please!