-
Notifications
You must be signed in to change notification settings - Fork 22
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
ExpandableText: Adds the expandable text component previously known as truncate component. #39
Conversation
@dlabaj we should not bring Sass support as we decided to use JSS for all of your components. Can you please migrate the styling to JSS? |
...y-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableTextExample.tsx
Show resolved
Hide resolved
.../content/extensions/component-groups/examples/ExpandableText/ExpandableTextInlineExample.tsx
Show resolved
Hide resolved
--- | ||
section: extensions | ||
subsection: Component groups | ||
id: ExpandableText |
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.
id: ExpandableText | |
id: Expandable text |
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.
Done.
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
@dlabaj posted some comments, overall looks good. Thank you 🙂 |
22fb5c8
to
3d9e585
Compare
...y-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableTextExample.tsx
Show resolved
Hide resolved
const text = `Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | ||
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | ||
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | ||
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | ||
laborum.`; |
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.
const text = `Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | |
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | |
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | |
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | |
laborum.`; | |
const text = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | |
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | |
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | |
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | |
laborum.'; |
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.
Unfortunatly this change doesn't work since the text spans multiple lines.
const text = `Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | ||
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | ||
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | ||
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | ||
laborum.`; |
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.
const text = `Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | |
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | |
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | |
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | |
laborum.`; | |
const text = 'Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore | |
et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea | |
commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla | |
pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est | |
laborum.'; |
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.
Tried the suggestion but we need the ` to allow the paragraph to run on multiple lines. Otherwise we would need to add '+ a the end of each line.
return inline ? ( | ||
<React.Fragment> | ||
<span className={expandableTextClasses} dangerouslySetInnerHTML={html} {...mouseOverHandler} {...props} /> | ||
{!hideExpandText && textOverflow && (showText === false ? expandButton : collapseButton)} |
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.
{!hideExpandText && textOverflow && (showText === false ? expandButton : collapseButton)} | |
{!hideExpandText && textOverflow && (showText ? collapseButton : expandButton)} |
...props | ||
}: ExpandableTextProps) => { | ||
const classes = useStyles(); | ||
const expandableTextClasses = clsx(classes.expandableText, className, { [`is-inline`]: inline }, { [`is-block`]: !inline }); |
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.
do we use is-block?
we could make these classes camelCase as well
|
||
const dangerousHtml = (html: string) => ({ __html: sanitizeHtml(html) }); | ||
|
||
export interface ExpandableTextCustomButtonProps { |
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.
thinking if this additional interface is needed... why not just add expandToggle and collapseToggle to ExpandableTextProps
?
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.
Because if they want to pass in a custom button they need to provide a collapse and expand button. However the custom button is optional in ExpandableTextProps. The documentation framework won't build if we put in a nested object in the props.
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.
lmk what you think
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
|
||
The **expandable text** component enables you to display long text to users via a tooltip. It builds off of the [tooltip component](https://www.patternfly.org/components/tooltip) to truncate UI text in an element and display the truncated text in a tooltip. | ||
|
||
### Basic ExpandableText component |
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.
### Basic ExpandableText component | |
### Basic expandable text |
|
||
## Component usage | ||
|
||
The **expandable text** component enables you to display long text to users via a tooltip. It builds off of the [tooltip component](https://www.patternfly.org/components/tooltip) to truncate UI text in an element and display the truncated text in a tooltip. |
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.
The **expandable text** component enables you to display long text to users via a tooltip. It builds off of the [tooltip component](https://www.patternfly.org/components/tooltip) to truncate UI text in an element and display the truncated text in a tooltip. | |
The **expandable text** component enables you to truncate long text, which can be viewed in full after expansion. |
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
### ExpandableText component with inline text | ||
|
||
This is an example of a ExpandableText component with inline text. It will truncate the text and display the full text when the button is clicked. The button will be display inline with the text and will toggle the text. |
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.
### ExpandableText component with inline text | |
This is an example of a ExpandableText component with inline text. It will truncate the text and display the full text when the button is clicked. The button will be display inline with the text and will toggle the text. | |
### With an inline button | |
To style the button inline with the truncated text, rather than below, pass in `inline={true}`. | |
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
...atternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md
Outdated
Show resolved
Hide resolved
|
||
import ExpandableText from "@patternfly/react-component-groups/dist/dynamic/ExpandableText"; | ||
|
||
## Component usage |
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.
## Component usage |
## Component usage | ||
|
||
The **expandable text** component enables you to truncate text and display the full text via a button. The button can be displayed inline with the text or below the text. The button can be customized to display custom text or a custom button. Alternatively you can display the full text on hover. | ||
|
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.
## Examples | |
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.
moving below the "intro" text
{button} | ||
</Button>; | ||
|
||
const expandButton = |
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 am in favor of splitting the expand button into a separate component and providing it as a default prop to a new expandButton
prop. Adding props API alongside a custom node element API is IMO a duplicate. If we split the expand button we give the consumer the ability to customize every aspect of the button.
They will have the ability to use a pre-defined variant, customize the props of the predefined variant by importing and changing its props, and use an entirely custom node if they choose to.
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 don't think we would need to make an expandButton component if we go this route. They could use a PF Button and pass that to use. Only thing the button did internally for the component is control showing the state, however if we are not going to control the state then I don't see the reason to create an expandButton component.
Thoughts?
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 don't think we would need to make an expandButton component if we go this route.
That is a valid point. Part of this is also checking if we actually need components like that. We have already identified several components that are "useless" and we will discontinue them from FEC. Maybe this component should be just a demo in some docs.
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.
@fhlavac brought this up as well last week if we would need this component. I think having a demo in some docs would work. Does that sound good?
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.
Regarding splitting the expand button into a separate component - In case we agree on keeping this component, I would say it's a good idea. The button component doesn't have to be (and probably should not be) a separate section in our docs, it can be still just part of ExpandableText docs.
Also agree that without the internal state, the component probably does not make sense to exist. So I would vote for either keeping the component with its internal state (use it or implement your own) or dropping it completely.
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.
@Hyperkid123 Thoughts on keeping state in and keeping this component?
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.
If we pull the button out we are going to have to take the state with it. Due to that I'm suggesting either getting rid of this component. I think a demo would be better.
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.
If I'm not mistaken, we wouldn't have to move the state to the button, it could only be consumed as a boolean flag according to which it would display its corresponding variant. When clicked, it would then just call the function of the parent component, which would change the state.
Anyway, it's getting rid of this component and creating a demo is probably the best solution. Talked with Martin and he is for dropping as well.
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.
Okay I'll close this PR, and get a demo put together somewhere.
const expandableTextClasses = clsx(classes.expandableText, className, { [`isInline`]: inline }); | ||
const trimmedText = text.substring(0, length); | ||
const textOverflow = text.length > length; | ||
const [ showText, setShowText ] = useState(false); |
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.
In general, we should try to have as little internal state management as possible. I would lift the expanded state out of this component.
It will simplify the component and the consumer will choose when and how to expand the text. We won't have to deal with cloning the buttons as that can be configured from the parent and with any other event. The logic can be also handled by completely different means.
Updated build:watch to build css and perform transform. Updated with comments. Fixed broken tests. fixed broken tests. fixed broken tests. Fixed export. Updated with review comments. Updated examples. Fixed import. Updated with correct documentation. Updated with review comments. Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableTextExample.tsx Co-authored-by: Filip Hlavac <50696716+fhlavac@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Update packages/module/patternfly-docs/content/extensions/component-groups/examples/ExpandableText/ExpandableText.md Co-authored-by: Erin Donehoo <105813956+edonehoo@users.noreply.github.com> Auto stash before checking out "origin/truncate"
Closing this PR in favor of doing a demo instead. |
Adds the expandableText component to react component groups.
https://expandedText.surge.sh