Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-folder-tree] Base Component Creation #1888

Merged
merged 15 commits into from
Nov 21, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
27 changes: 16 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions packages/terra-folder-tree/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

## Unreleased

* Added
* Created base `terra-folder-tree` component.

## 0.1.1-alpha.0 - (October 25, 2023)

* Changed
* Minor dependency version bump

* Minor dependency version bump.
7 changes: 6 additions & 1 deletion packages/terra-folder-tree/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@
"dependencies": {
"classnames": "^2.2.5",
"keycode-js": "^3.1.0",
"prop-types": "^15.5.8"
"prop-types": "^15.5.8",
"terra-action-header": "^2.85.0",
"terra-arrange": "^3.54.0",
"terra-icon": "^3.58.0",
"terra-spacer": "^3.61.0",
"terra-theme-context": "^1.8.0"
},
"devDependencies": {
"@babel/cli": "^7.22.10",
Expand Down
46 changes: 35 additions & 11 deletions packages/terra-folder-tree/src/FolderTree.jsx
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,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

/**
* 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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

@sdadn sdadn Nov 17, 2023

Choose a reason for hiding this comment

The 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;
10 changes: 7 additions & 3 deletions packages/terra-folder-tree/src/FolderTree.module.scss
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: ;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you might need to remove this commented code.

}
122 changes: 122 additions & 0 deletions packages/terra-folder-tree/src/subcomponents/FolderTreeItem.jsx
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.
JessieRandle marked this conversation as resolved.
Show resolved Hide resolved
*/
subfolderItems: PropTypes.arrayOf(PropTypes.element),
/**
* Whether or not the item is selected.
JessieRandle marked this conversation as resolved.
Show resolved Hide resolved
*/
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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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` }}>
Copy link
Contributor

Choose a reason for hiding this comment

The 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>
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With break-word, long strings without spaces do not wrap which is why I used anywhere, do you know of a way to handle that with break-word?

image

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%));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the focus state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
}
}
Loading
Loading