nytai commented on a change in pull request #10607:
URL:
https://github.com/apache/incubator-superset/pull/10607#discussion_r471889133
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -154,12 +155,13 @@ function ListViewCard({
coverLeft,
coverRight,
actions,
+ bulkSelectEnabled,
}: CardProps) {
return (
<StyledCard
cover={
<Cover>
- <a href={url}>
+ <a href={bulkSelectEnabled ? undefined : url}>
Review comment:
Alternatively, we may just be able to do a
```js
e.preventDefault();
e.stopPropagation();
```
in the CardWrapper's onClick handler, when bulkSelect is enabled, to prevent
events from triggering on the children.
##########
File path: superset-frontend/src/components/ListViewCard/index.tsx
##########
@@ -154,12 +155,13 @@ function ListViewCard({
coverLeft,
coverRight,
actions,
+ bulkSelectEnabled,
}: CardProps) {
return (
<StyledCard
cover={
<Cover>
- <a href={url}>
+ <a href={bulkSelectEnabled ? undefined : url}>
Review comment:
can we move this logic up into the `renderCard` functions? We can make
url optional and not pass it down when bulkSelect is enabled, that way this
component doesn't need to know about bulk select.
##########
File path: superset-frontend/src/components/ListView/CardCollection.tsx
##########
@@ -36,19 +37,36 @@ const CardContainer = styled.div`
${({ theme }) => theme.gridUnit * 4}px;
`;
+const CardWrapper = styled.div`
+ border: 2px solid transparent;
+ &.card-selected {
+ border: 2px solid ${({ theme }) => theme.colors.primary.base};
+ }
+`;
+
export default function CardCollection({
- renderCard,
+ bulkSelectEnabled,
+ loading,
prepareRow,
+ renderCard,
rows,
- loading,
-}: Props) {
+}: CardCollectionProps) {
return (
<CardContainer>
{rows.map(row => {
if (!renderCard) return null;
prepareRow(row);
return (
- <div key={row.id}>{renderCard({ ...row.original, loading })}</div>
+ <CardWrapper
+ className={
+ row.isSelected && bulkSelectEnabled ? 'card-selected' : ''
+ }
+ key={row.id}
+ onClick={() => row.toggleRowSelected()}
Review comment:
We should probably check to see if `bulkSelectEnabled` before firing
this. Like this, I think it's possible to select cards even if bulkSelect is
not enabled.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
