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.

Sign up for GitHub

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

refactor(CodeSnippet): use hooks clean up render logic #4636

Merged
merged 18 commits into from
Nov 16, 2019
Merged

refactor(CodeSnippet): use hooks clean up render logic #4636

Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9f8c4b7
refactor(CodeSnippet): use hooks clean up render logic
vpicone Nov 9, 2019
dfdaf96
Update packages/react/src/components/CodeSnippet/CodeSnippet.js
vpicone Nov 11, 2019
e76c476
fix: update when children or ref changes
vpicone Nov 11, 2019
61b21d4
Merge branch 'code-snippet-refactor' of github.com:vpicone/carbon int…
vpicone Nov 11, 2019
cca649f
refactor: organize
vpicone Nov 11, 2019
e6d051f
Merge branch 'master' into code-snippet-refactor
vpicone Nov 11, 2019
279ce84
fix: use function rather than arrow
vpicone Nov 11, 2019
3b9582c
Merge branch 'code-snippet-refactor' of github.com:vpicone/carbon int…
vpicone Nov 11, 2019
2d00460
fix: ref destructuring
vpicone Nov 12, 2019
2a35126
Merge branch 'master' into code-snippet-refactor
vpicone Nov 12, 2019
9d742d8
Merge branch 'master' into code-snippet-refactor
vpicone Nov 12, 2019
803b29d
Update packages/react/src/components/CodeSnippet/CodeSnippet.js
vpicone Nov 13, 2019
0882caf
fix: spread variable
vpicone Nov 13, 2019
1deb7ed
Merge branch 'master' into code-snippet-refactor
vpicone Nov 14, 2019
0d69f32
Merge branch 'master' into code-snippet-refactor
joshblack Nov 15, 2019
73eef6c
Merge branch 'master' into code-snippet-refactor
vpicone Nov 15, 2019
61aff34
Merge branch 'master' into code-snippet-refactor
vpicone Nov 15, 2019
da0cea6
Merge branch 'master' into code-snippet-refactor
vpicone Nov 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
333 changes: 140 additions & 193 deletions packages/react/src/components/CodeSnippet/CodeSnippet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,223 +6,170 @@
*/

import PropTypes from 'prop-types';
import React, { Component } from 'react';
import React, { useState, useRef, useLayoutEffect } from 'react';
import classNames from 'classnames';
import { ChevronDown16 } from '@carbon/icons-react';
import { settings } from 'carbon-components';
import Copy from '../Copy';
import Button from '../Button';
import CopyButton from '../CopyButton';
import uid from '../../tools/uniqueId';
import getUniqueId from '../../tools/uniqueId';

const { prefix } = settings;

export default class CodeSnippet extends Component {
static propTypes = {
/**
* Provide the type of Code Snippet
*/
type: PropTypes.oneOf(['single', 'inline', 'multi']),

/**
* Specify an optional className to be applied to the container node
*/
className: PropTypes.string,

/**
* Provide the content of your CodeSnippet as a string
*/
children: PropTypes.string,

/**
* Specify the string displayed when the snippet is copied
*/
feedback: PropTypes.string,

/**
* Specify the description for the Copy Button
*/
copyButtonDescription: PropTypes.string,

/**
* An optional handler to listen to the `onClick` even fired by the Copy
* Button
*/
onClick: PropTypes.func,

/**
* Specify a label to be read by screen readers on the containing <textbox>
* node
*/
copyLabel: PropTypes.string,

/**
* Specify a label to be read by screen readers on the containing <textbox>
* node
*/
ariaLabel: PropTypes.string,

/**
* Specify a string that is displayed when the Code Snippet text is more
* than 15 lines
*/
showMoreText: PropTypes.string,

/**
* Specify a string that is displayed when the Code Snippet has been
* interacted with to show more lines
*/
showLessText: PropTypes.string,

/**
* Specify whether you are using the light variant of the Code Snippet,
* typically used for inline snippet to display an alternate color
*/
light: PropTypes.bool,
};

static defaultProps = {
type: 'single',
showMoreText: 'Show more',
showLessText: 'Show less',
};

state = {
shouldShowMoreLessBtn: false,
expandedCode: false,
};

componentDidMount() {
if (this.codeContent) {
if (this.codeContent.getBoundingClientRect().height > 255) {
this.setState({ shouldShowMoreLessBtn: true });
}
function CodeSnippet({
className,
type,
children,
feedback,
onClick,
ariaLabel,
copyLabel, //TODO: Merge this prop to `ariaLabel` in `v11`
copyButtonDescription,
light,
showMoreText,
showLessText,
...other
}) {
const [expandedCode, setExpandedCode] = useState(false);
const [shouldShowMoreLessBtn, setShouldShowMoreLessBtn] = useState(false);
const { current: uid } = useRef(getUniqueId());
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that I don't think we remember is that technically we should not have ids start with a number, I think it's fine in HTML5 but in general can cause issues in certain cases (or at least has been brought up that way). Would we want to bring over our useId hook or add a prefix to this that's been passed to id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshblack our uniqueId actually adds an id prefix by default, so starting with a number shouldn't be an issue. Do you think we should be specifying a more descriptive prefix? I'm a little gun shy about tweaking ID generation behavior at this point haha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't realize it was using the older file for unique ids. In general, we use https://github.com/carbon-design-system/carbon/blob/master/packages/react/src/tools/setupGetInstanceId.js for newer code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshblack oh missed that. I think this is a great opportunity to switch to a hook. I really highly doubt someone's depending on the ID of the inline code component, but do you think it'd be worth keeping the id prefix?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that an external user is leveraging it, just that folks have pointed out that ids generated with a number as the first character technically doesn't adhere to the HTML4 spec (I think this is addressed in HTML5). I think we could totally do id-{number}, we just have to be careful around collisions and the reason this came up with the newer approach is that it's easy to collide since they don't all share the same global id variable (which helps out a ton for tests). I think combining the local prefix plus the global id generation in a hook would be great and useId could help do that

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 opened #4676 since I feel like changing the id generation behavior might be a little beyond the scope. The hook we have in the hooks package forces a - that doesn't exist in the current uniqueId implementation so it'd break some snapshots probably.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vpicone are we or others using automatically generated ids in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshblack wasn’t that the impetus for that big slack thread? Someone was using a generated ID to select an element in testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vpicone that was when someone used an id they passed into the component, not a generated one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh!!! That makes way more sense.

const codeContentRef = useRef();

useLayoutEffect(() => {
if (codeContentRef.current) {
const { height } = codeContentRef.current.getBoundingClientRect();
setShouldShowMoreLessBtn(type === 'multi' && height > 255);
}
}
}, [children, type]);

componentDidUpdate(prevProps) {
if (this.props.children !== prevProps.children && this.codeContent) {
if (this.codeContent.getBoundingClientRect().height > 255) {
this.setState({ shouldShowMoreLessBtn: true });
}
}
}
const codeSnippetClasses = classNames(className, {
[`${prefix}--snippet`]: true,
[`${prefix}--snippet--${type}`]: type,
[`${prefix}--snippet--expand`]: expandedCode,
[`${prefix}--snippet--light`]: light,
});

expandCode = () => {
this.setState({ expandedCode: !this.state.expandedCode });
};

render() {
const {
className,
type,
children,
feedback,
onClick,
ariaLabel,
copyLabel, //TODO: Merge this prop to `ariaLabel` in `v11`
copyButtonDescription,
light,
showMoreText,
showLessText,
...other
} = this.props;

// a unique id generated for aria-describedby
this.uid = uid();

const codeSnippetClasses = classNames(className, {
[`${prefix}--snippet`]: true,
[`${prefix}--snippet--${type}`]: type,
[`${prefix}--snippet--expand`]: this.state.expandedCode,
[`${prefix}--snippet--light`]: light,
});

const expandCodeBtnText = this.state.expandedCode
? showLessText
: showMoreText;

const moreLessBtn = (
<button
className={`${prefix}--btn ${prefix}--btn--ghost ${prefix}--btn--sm ${prefix}--snippet-btn--expand`}
type="button"
onClick={this.expandCode}>
<span className={`${prefix}--snippet-btn--text`}>
{expandCodeBtnText}
</span>
<ChevronDown16
aria-label={expandCodeBtnText}
className={`${prefix}--icon-chevron--down ${prefix}--snippet__icon`}
name="chevron--down"
role="img"
/>
</button>
const expandCodeBtnText = expandedCode ? showLessText : showMoreText;

if (type === 'inline') {
return (
<Copy
{...other}
onClick={onClick}
aria-label={copyLabel || ariaLabel}
aria-describedby={uid}
className={codeSnippetClasses}
feedback={feedback}>
<code id={uid}>{children}</code>
</Copy>
);
}

const code = (
return (
<div {...other} className={codeSnippetClasses}>
<div
role="textbox"
tabIndex={0}
className={`${prefix}--snippet-container`}
aria-label={ariaLabel || copyLabel || 'code-snippet'}>
<code>
<pre
ref={codeContent => {
this.codeContent = codeContent;
}}>
{children}
</pre>
<pre ref={codeContentRef}>{children}</pre>
</code>
</div>
);

const copy = (
<CopyButton
onClick={onClick}
feedback={feedback}
iconDescription={copyButtonDescription}
/>
);

if (type === 'inline') {
return (
<Copy
{...other}
onClick={onClick}
aria-label={copyLabel || ariaLabel}
aria-describedby={this.uid}
className={codeSnippetClasses}
feedback={feedback}>
<code id={this.uid}>{children}</code>
</Copy>
);
}

if (type === 'single') {
return (
<div {...other} className={codeSnippetClasses}>
{code}
{copy}
</div>
);
}

if (!this.state.shouldShowMoreLessBtn && type === 'multi') {
return (
<div {...other} className={codeSnippetClasses}>
{code}
{copy}
</div>
);
}

if (this.state.shouldShowMoreLessBtn && type === 'multi') {
return (
<div className={codeSnippetClasses} {...other}>
{code}
{copy}
{moreLessBtn}
</div>
);
}
}
{shouldShowMoreLessBtn && (
<Button
kind="ghost"
size="small"
className={`${prefix}--snippet-btn--expand`}
onClick={() => setExpandedCode(!expandedCode)}>
<span className={`${prefix}--snippet-btn--text`}>
{expandCodeBtnText}
</span>
<ChevronDown16
aria-label={expandCodeBtnText}
className={`${prefix}--icon-chevron--down ${prefix}--snippet__icon`}
name="chevron--down"
role="img"
/>
</Button>
)}
</div>
);
}

CodeSnippet.propTypes = {
/**
* Provide the type of Code Snippet
*/
type: PropTypes.oneOf(['single', 'inline', 'multi']),

/**
* Specify an optional className to be applied to the container node
*/
className: PropTypes.string,

/**
* Provide the content of your CodeSnippet as a string
*/
children: PropTypes.string,

/**
* Specify the string displayed when the snippet is copied
*/
feedback: PropTypes.string,

/**
* Specify the description for the Copy Button
*/
copyButtonDescription: PropTypes.string,

/**
* An optional handler to listen to the `onClick` even fired by the Copy
* Button
*/
onClick: PropTypes.func,

/**
* Specify a label to be read by screen readers on the containing <textbox>
* node
*/
copyLabel: PropTypes.string,

/**
* Specify a label to be read by screen readers on the containing <textbox>
* node
*/
ariaLabel: PropTypes.string,

/**
* Specify a string that is displayed when the Code Snippet text is more
* than 15 lines
*/
showMoreText: PropTypes.string,

/**
* Specify a string that is displayed when the Code Snippet has been
* interacted with to show more lines
*/
showLessText: PropTypes.string,

/**
* Specify whether you are using the light variant of the Code Snippet,
* typically used for inline snippet to display an alternate color
*/
light: PropTypes.bool,
};

CodeSnippet.defaultProps = {
type: 'single',
showMoreText: 'Show more',
showLessText: 'Show less',
};

export default CodeSnippet;