-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add multi-select action #1319
Add multi-select action #1319
Conversation
Published tno-core:0.0.384
/** whether or not this is being used on a list view */ | ||
onList?: boolean; | ||
/** Whether the back button is displayed */ | ||
showBackButton?: boolean; |
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.
Make components that are not tightly coupled to implementation if possible.
@@ -0,0 +1,5 @@ | |||
import { ContentActionBar, IContentActionBarProps } from './ContentActionBar'; | |||
|
|||
export const ContentListActionBar: React.FC<IContentActionBarProps> = ({ className, ...rest }) => { |
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.
Since a common implementation is to add the className, we can create a simple wrapper.
@@ -185,7 +185,7 @@ export const ViewContent: React.FC<IViewContentProps> = ({ setActiveContent }) = | |||
<div className="source-section">{`${content?.section} ${ | |||
content?.page && `:${content.page}` | |||
}`}</div> | |||
{content?.tonePools && ( | |||
{content?.tonePools && content?.tonePools.length && ( |
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.
Fixed bug unrelated to this PR. Developers please don't assume an array has items in it.
@@ -33,28 +33,32 @@ export const Home: React.FC = () => { | |||
}, | |||
{ findContentWithElasticsearch, storeHomeFilter: storeFilter }, | |||
] = useContent(); | |||
const [homeItems, setHomeItems] = React.useState<IContentModel[]>([]); | |||
const navigate = useNavigate(); |
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.
Keep the organization convention alive. It's easier for all of us to read the code.
Place dependency hooks first.
Then React.useState
Then local variables
Then React.useEffect
and functions
@@ -70,8 +70,7 @@ export class TableInternal<T extends object> implements ITableInternal<T> { | |||
const rows = this.filterData(this.search).map((original, index) => { | |||
const selected = this.selectedRowIds.some((id) => this._rowId(original) === id) | |||
? true | |||
: selectedRows.find((row) => this._rowId(row.original) === this._rowId(original)) | |||
?.isSelected ?? false; | |||
: 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.
This was a bug, but I don't know whether this will break something else.
/** Event fires when back button is pressed */ | ||
onBack?: () => void; | ||
/** Event fires when select all checkbox is changed */ | ||
onSelectAll?: React.ChangeEventHandler<HTMLInputElement>; |
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.
Use an event to let the parent decide on implementation.
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 like it, thanks for doing that. I needed fresh eyes
Cleaned up component and page implementation to support a toolbar action to select all content.
@jdtoombs You can figure out how to make it look better. The select all highlights each row which is ugly.
Summary
List View