[WC-3088] DataGrid: Row selection indicator #1907
Conversation
yordan-st
changed the title
yordan-st
force-pushed
the
fix/wc-3088_datagrid-row-selection
branch
2 times, most recently
from
fa68cf4 to
c7622d9
Compare
yordan-st
force-pushed
the
fix/wc-3088_datagrid-row-selection
branch
2 times, most recently
from
72b5ccc to
07c86b0
Compare
yordan-st
force-pushed
the
fix/wc-3088_datagrid-row-selection
branch
from
07c86b0 to
afb73a8
Compare
| type WidgetTopBarProps = { | ||
| selectionCountVisibility?: SelectionCountVisibilityEnum; | ||
| clearSelectionButtonLabel?: string; | ||
| showTopBar: boolean; |
There was a problem hiding this comment.
This name make no sense, it is not about topBar, but about showing or not showing pagination part of it.
| {props.children} | ||
| <div {...restProps} className="widget-datagrid-top-bar table-header"> | ||
| <div className="widget-datagrid-padding-top"> | ||
| {selectionCountVisibility === "top" && selectedCount > 0 && ( |
There was a problem hiding this comment.
This check can be move outside of the component can receive just a boolean.
| <div className="widget-datagrid-padding-top"> | ||
| {selectionCountVisibility === "top" && selectedCount > 0 && ( | ||
| <div className="widget-datagrid-tb-start"> | ||
| <SelectionCounter clearSelectionButtonLabel={clearSelectionButtonLabel} /> |
There was a problem hiding this comment.
I think it is better to pass this same way as we pass pagination (as a prop myProperty={<MyComponent />}). This we we don't need to pass along props that are needed for SelectionCounter only.
Arguably pagination has to be passed along the similar way, so it does look unified.
Also maybe instead of passing booleans we can just check if prop exists, like:
{props.myProperty && <div className="widget-datagrid-tb-end">{props.myProperty}</div>}
This way if we pass null it won't render.
There was a problem hiding this comment.
For example: see how we pass pagination to WidgetFooter.
| <enumerationValue key="both">Both</enumerationValue> | ||
| </enumerationValues> | ||
| </property> | ||
| <property key="selectionCountVisibility" type="enumeration" defaultValue="bottom" required="true"> |
There was a problem hiding this comment.
We already have setting called pagingPosition, which basically controls almost identical behavior but for pagination. To make it consistent I suggest naming this one somethingSomethingPosition as well.
| pageSize={props.pageSize} | ||
| paginationType={props.pagination} | ||
| loadMoreButtonCaption={props.loadMoreButtonCaption?.value} | ||
| clearSelectionButtonLabel={props.clearSelectionButtonLabel?.value} |
There was a problem hiding this comment.
Maybe this one should be part of SelectionCountStore, so <SelectionCounter /> can use it from the store, so we don't have to drill this property down through the widget.
| const showTopBar = paging && (pagingPosition === "top" || pagingPosition === "both"); | ||
| const showFooter = paging && (pagingPosition === "bottom" || pagingPosition === "both"); |
There was a problem hiding this comment.
They have to be named correctly, something like showTopBarPagination and showFooterPagination as after the change, pagination is not the only part that determines if the topbar/footer is shown, so the names have to be adjusted.
| </div> | ||
| {hasMoreItems && paginationType === "loadMore" && ( | ||
| <div className="widget-datagrid-pb-middle"> | ||
| {selectionCountVisibility === "bottom" && selectedCount > 0 && ( |
There was a problem hiding this comment.
All comments for WidgetTopBar apply here as well.
Pull request type
Bug fix (non-breaking change which fixes an issue)
Description
What should be covered while testing?