-
Notifications
You must be signed in to change notification settings - Fork 72
[terra-folder-tree] Base Component Creation #1888
Changes from all commits
66de3ee
dc8e3a3
cecce12
98c075c
58a0c2b
bf4b905
bcef3b5
b99e6b8
ad7a80b
625d530
1a926fe
2e28645
ce601d7
bbc0556
89a57bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,50 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { injectIntl } from 'react-intl'; | ||
// import classNames from 'classnames/bind'; | ||
// import styles from './FolderTree.module.scss'; | ||
import classNames from 'classnames/bind'; | ||
|
||
// const cx = classNames.bind(styles); | ||
import ActionHeader from 'terra-action-header'; | ||
|
||
import FolderTreeItem from './subcomponents/FolderTreeItem'; | ||
import styles from './FolderTree.module.scss'; | ||
|
||
const cx = classNames.bind(styles); | ||
|
||
const propTypes = { | ||
/** | ||
* @private | ||
* The intl object containing translations. This is retrieved from the context automatically by injectIntl. | ||
* List of FolderTree.Items to be displayed as content within the FolderTree. | ||
*/ | ||
children: PropTypes.node.isRequired, | ||
/** | ||
* The title of the folder tree. | ||
*/ | ||
intl: PropTypes.shape({ formatMessage: PropTypes.func }).isRequired, | ||
title: PropTypes.string.isRequired, | ||
/** | ||
* The heading level for the title of the folder tree. | ||
*/ | ||
headerLevel: PropTypes.number, | ||
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. Is this a value that we want to be configurable or should it be a constant value for all FolderTree components. 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. Talked with @eawww, we want to keep the header level configurable since we don't know where on a page FolderTree will be used so consumers will need the option to set custom heading levels. |
||
}; | ||
|
||
const defaultProps = {}; | ||
const defaultProps = { | ||
headerLevel: 3, | ||
}; | ||
|
||
const FolderTree = () => ( | ||
<div>test</div> | ||
const FolderTree = ({ children, title, headerLevel }) => ( | ||
<> | ||
<ActionHeader | ||
text={title} | ||
level={headerLevel} | ||
/> | ||
<ul | ||
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. This needs an aria-labeledby pointing to the header above it or an aria-label with the same text. |
||
className={cx('folder-tree')} | ||
role="tree" | ||
> | ||
{children} | ||
</ul> | ||
</> | ||
); | ||
|
||
FolderTree.propTypes = propTypes; | ||
FolderTree.defaultProps = defaultProps; | ||
|
||
export default injectIntl(FolderTree); | ||
FolderTree.Item = FolderTreeItem; | ||
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. Can we do this in an index.js file since there are multiple exports? |
||
export default FolderTree; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,11 @@ | ||
@import '~terra-mixins/lib/Mixins'; | ||
|
||
// Themes | ||
@import './orion-fusion-theme/FolderTree.module'; | ||
@import './clinical-lowlight-theme/FolderTree.module'; | ||
|
||
// :local {} | ||
:local { | ||
.folder-tree { | ||
list-style: none; | ||
margin: 0; | ||
padding: 0; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
// :local { | ||
// .clinical-lowlight-theme {} | ||
// } | ||
:local { | ||
.clinical-lowlight-theme { | ||
--terra-folder-tree-item-border-bottom: #181b1d; | ||
--terra-folder-tree-item-background-color: #1e3a49; | ||
--terra-folder-tree-item-hover-background-color: #232a2d; | ||
--terra-folder-tree-item-selected-background-color: #232a2d; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
// :local { | ||
// .orion-fusion-theme {} | ||
// } | ||
:local { | ||
.orion-fusion-theme { | ||
--terra-folder-tree-item-background-color-hover: #f4fafe; | ||
// --terra-folder-tree-item-background-color-selected: ; | ||
} | ||
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. Looks like you might need to remove this commented code. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,122 @@ | ||
import React, { useContext } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import classNames from 'classnames/bind'; | ||
import { injectIntl } from 'react-intl'; | ||
|
||
import Spacer from 'terra-spacer'; | ||
import Arrange from 'terra-arrange'; | ||
import { IconFolder } from 'terra-icon'; | ||
import ThemeContext from 'terra-theme-context'; | ||
|
||
import styles from './FolderTreeItem.module.scss'; | ||
|
||
const cx = classNames.bind(styles); | ||
|
||
const propTypes = { | ||
/** | ||
* The label for the folder tree item. | ||
*/ | ||
label: PropTypes.string.isRequired, | ||
/** | ||
* The icon to display to the left of the name. | ||
*/ | ||
icon: PropTypes.element, | ||
/** | ||
* List of FolderTree.Items to display in a subfolder when this FolderTreeItem is clicked. This makes the item expandable and collapsible. It also makes the item's icon a Folder, overriding anything given as the icon prop. | ||
*/ | ||
subfolderItems: PropTypes.arrayOf(PropTypes.element), | ||
/** | ||
* Whether or not the item is selected. Since this component has the appearance of a radio button group, only one item should be selected at a time. | ||
*/ | ||
isSelected: PropTypes.bool, | ||
/** | ||
* Callback function for click event. | ||
*/ | ||
onClick: PropTypes.func, | ||
/** | ||
* @private | ||
* Level of nesting for this item. | ||
*/ | ||
level: PropTypes.number, | ||
/** | ||
* @private | ||
* intl object programmatically imported through injectIntl from react-intl. | ||
* */ | ||
intl: PropTypes.shape({ formatMessage: PropTypes.func }).isRequired, | ||
}; | ||
|
||
const defaultProps = { | ||
isSelected: false, | ||
level: 0, | ||
}; | ||
|
||
const FolderTreeItem = ({ | ||
icon, | ||
label, | ||
subfolderItems, | ||
isSelected, | ||
onClick, | ||
level, | ||
intl, | ||
}) => { | ||
const theme = useContext(ThemeContext); | ||
|
||
const subfolder = subfolderItems?.length > 0 ? ( | ||
<ul | ||
className={cx('subfolder')} | ||
role="group" | ||
> | ||
{subfolderItems.map((item) => ( | ||
<FolderTreeItem | ||
{...item.props} | ||
intl={intl} | ||
level={level + 1} | ||
/> | ||
))} | ||
</ul> | ||
) : null; | ||
|
||
const itemIcon = subfolder ? <IconFolder a11yLabel={intl.formatMessage({ id: 'Terra.folder-tree.folder-icon' })} /> : icon; | ||
|
||
const itemClassNames = classNames( | ||
cx( | ||
'folder-tree-item', | ||
{ selected: isSelected }, | ||
theme.className, | ||
), | ||
); | ||
|
||
return ( | ||
<> | ||
<li | ||
className={itemClassNames} | ||
role="treeitem" | ||
aria-selected={isSelected} | ||
> | ||
<input | ||
type="radio" | ||
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 shouldn't be radio buttons since they can't be properly associated with each other and we'll be using aria-selected instead. |
||
checked={isSelected} | ||
onClick={onClick} | ||
/> | ||
{/* eslint-disable-next-line react/forbid-dom-props */} | ||
<span style={{ paddingLeft: `${level}rem` }}> | ||
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. Why are we adding a span element around the Terra Arrange? Are we trying to use this like the Spacer component? |
||
<Arrange | ||
fitStart={( | ||
<Spacer paddingLeft="medium" paddingRight="medium" isInlineBlock> | ||
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. It looks like we are using both a span and Spacer element to add padding. |
||
{itemIcon} | ||
</Spacer> | ||
)} | ||
fill={label} | ||
alignFitStart="center" | ||
/> | ||
</span> | ||
Comment on lines
+103
to
+112
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. Instead of adding this complexity. Can't we just use the listStyleImage property to set the image? Also, are we able to leverage list-style-position to assist with the indentation? Can we maybe limit the custom styling necessary? |
||
</li> | ||
{subfolder} | ||
</> | ||
); | ||
}; | ||
|
||
FolderTreeItem.propTypes = propTypes; | ||
FolderTreeItem.defaultProps = defaultProps; | ||
|
||
export default injectIntl(FolderTreeItem); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
@import '../clinical-lowlight-theme/FolderTree.module'; | ||
@import '../orion-fusion-theme/FolderTree.module'; | ||
|
||
:local { | ||
.folder-tree-item { | ||
align-items: center; | ||
background-color: var(--terra-folder-tree-item-background-color, #fff); | ||
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 think it is better to use background instead of background-color. The background style would allow for both colors and gradients without a future style change. |
||
border-bottom: var(--terra-folder-tree-item-border-bottom, 1px solid #dedfe0); | ||
display: flex; | ||
margin: 0; | ||
min-height: var(--terra-folder-tree-item-min-height, 2.92857rem); | ||
overflow-wrap: anywhere; | ||
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. We would want this to be "break-word". I don't think we want to use anywhere as our option. https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap 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. |
||
padding: 0; | ||
width: 100%; | ||
|
||
&:hover { | ||
background-color: var(--terra-folder-tree-item-hover-background-color, #f4fafe); | ||
} | ||
} | ||
|
||
.selected { | ||
background-color: var(--terra-folder-tree-item-selected-background-color, darken(#ebf6fd, 2%)); | ||
} | ||
|
||
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. What about the focus state? 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. The focus state should be added with keyboard navigation (UXPLATFORM-9776). |
||
.subfolder { | ||
list-style: none; | ||
margin: 0; | ||
padding: 0; | ||
} | ||
} |
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.
Will there be any issues to making this required? What if a consumer does
<FolderTree><FolderTree/>
and dynamically adds the children later?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.
My thought in marking this as required was that without any FolderTreeItems, it's really not a folder tree at all but a floating header. But it doesn't really hurt to make this optional since it's up to consumers what content to put in the tree.