Skip to content

fix(contentpicker): correctly clear old item when maxSelectable=1 and parent reloaded #1706

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.

Sign up for GitHub

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

Open
cgrdavies wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix(contentpicker): correctly clear old item when maxSelectable=1 and parent reloaded #1706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
663eb81
fix(content-picker): fix issues; see if tests pass
cgrdavies Nov 7, 2019
b913950
fix(content-picker): factor out commonalities
cgrdavies Nov 7, 2019
4c32a45
Merge branch 'master' of github.com:box/box-ui-elements into fix-sele…
cgrdavies Nov 7, 2019
4e67b38
fix(content-picker): fix linting in checkboxcellrenderer
cgrdavies Nov 7, 2019
5d5ed70
Merge branch 'master' of github.com:box/box-ui-elements into fix-sing…
cgrdavies Nov 12, 2019
5a28b52
fix(content-picker): Pr Feedback
cgrdavies Nov 13, 2019
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
3 changes: 3 additions & 0 deletions src/elements/content-picker/Content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type Props = {
rootElement?: HTMLElement,
rootId: string,
selectableType: string,
selected: Array<BoxItem>,
tableRef: Function,
view: View,
};
Expand Down Expand Up @@ -60,6 +61,7 @@ const Content = ({
onShareAccessChange,
onFocusChange,
extensionsWhitelist,
selected,
}: Props) => (
<div className="bcp-content">
{view === VIEW_ERROR || view === VIEW_SELECTED ? null : (
Expand All @@ -85,6 +87,7 @@ const Content = ({
onFocusChange={onFocusChange}
onShareAccessChange={onShareAccessChange}
extensionsWhitelist={extensionsWhitelist}
selected={selected}
/>
)}
</div>
Expand Down
110 changes: 52 additions & 58 deletions src/elements/content-picker/ContentPicker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import Internationalize from '../common/Internationalize';
import makeResponsive from '../common/makeResponsive';
import Pagination from '../common/pagination/Pagination';
import { isFocusableElement, isInputElement, focus } from '../../utils/dom';
import { isSelected, containsItem } from './itemSelectionHelper';
import API from '../../api';
import Content from './Content';
import Footer from './Footer';
Expand Down Expand Up @@ -102,7 +103,7 @@ type State = {
isUploadModalOpen: boolean,
rootName: string,
searchQuery: string,
selected: { [string]: BoxItem },
selected: Array<BoxItem>,
sortBy: SortBy,
sortDirection: SortDirection,
view: View,
Expand Down Expand Up @@ -199,7 +200,7 @@ class ContentPicker extends Component<Props, State> {
currentCollection: {},
currentOffset: initialPageSize * (initialPage - 1),
currentPageSize: initialPageSize,
selected: {},
selected: [],
searchQuery: '',
view: VIEW_FOLDER,
isCreateFolderModalOpen: false,
Expand Down Expand Up @@ -287,11 +288,10 @@ class ContentPicker extends Component<Props, State> {
choose = (): void => {
const { selected }: State = this.state;
const { onChoose }: Props = this.props;
const results: BoxItem[] = Object.keys(selected).map(key => {
const clone: BoxItem = { ...selected[key] };
delete clone.selected;
return clone;
const results: BoxItem[] = selected.map(selectedItem => {
return { ...selectedItem };
});

onChoose(results);
};

Expand All @@ -304,13 +304,9 @@ class ContentPicker extends Component<Props, State> {
*/
cancel = (): void => {
const { onCancel }: Props = this.props;
const { selected }: State = this.state;

// Clear out the selected field
Object.keys(selected).forEach(key => delete selected[key].selected);

// Reset the selected state
this.setState({ selected: {} }, () => onCancel());
this.setState({ selected: [] }, () => onCancel());
};

/**
Expand Down Expand Up @@ -563,7 +559,7 @@ class ContentPicker extends Component<Props, State> {
sortBy,
sortDirection,
percentLoaded: 100,
items: Object.keys(selected).map(key => this.api.getCache().get(key)),
items: selected,
},
},
this.finishNavigation,
Expand Down Expand Up @@ -782,46 +778,24 @@ class ContentPicker extends Component<Props, State> {
return;
}

const selectedKeys: Array<string> = Object.keys(selected);
const selectedCount: number = selectedKeys.length;
const hasHitSelectionLimit: boolean = selectedCount === maxSelectable;
const isSingleFileSelection: boolean = maxSelectable === 1;
const cacheKey: string = this.api.getAPI(type).getCacheKey(id);
const existing: BoxItem = selected[cacheKey];
const existingFromCache: BoxItem = this.api.getCache().get(cacheKey);
const existInSelected = selectedKeys.indexOf(cacheKey) !== -1;
const newSelected: Array<BoxItem> = isSingleFileSelection ? [] : [...selected];
const selectedCount: number = newSelected.length;
const hasHitSelectionLimit: boolean = selectedCount === maxSelectable;
const existingIndex: number = newSelected.findIndex(containsItem(item));
const isAlreadySelected = existingIndex !== -1 || (isSingleFileSelection && isSelected(item, selected));
const itemCanSetShareAccess = getProp(item, 'permissions.can_set_share_access', false);

// Existing object could have mutated and we just need to update the
// reference in the selected map. In that case treat it like a new selection.
if (existing && existing === existingFromCache) {
if (isAlreadySelected) {
// We are selecting the same item that was already
// selected. Unselect it in this case. Toggle case.
delete existing.selected;
delete selected[cacheKey];
newSelected.splice(existingIndex, 1);
} else {
// We are selecting a new item that was never
// selected before. However if we are in a single
// item selection mode, we should also unselect any
// prior item that was item that was selected.

// Check if we hit the selection limit and if selection
// is not already currently in the selected data structure.
// Ignore when in single file selection mode.
if (hasHitSelectionLimit && !isSingleFileSelection && !existInSelected) {
if (hasHitSelectionLimit) {
return;
}

// Clear out the prior item for single file selection mode
if (selectedCount > 0 && isSingleFileSelection) {
const prior = selectedKeys[0]; // only one item
delete selected[prior].selected;
delete selected[prior];
}

// Select the new item
item.selected = true;
selected[cacheKey] = item;
newSelected.push(item);

// If can set share access, fetch the shared link properties of the item
// In the case where another item is selected, any in flight XHR will get
Expand All @@ -832,7 +806,7 @@ class ContentPicker extends Component<Props, State> {
}

const focusedRow = items.findIndex((i: BoxItem) => i.id === item.id);
this.setState({ selected, focusedRow }, () => {
this.setState({ selected: newSelected, focusedRow }, () => {
if (view === VIEW_SELECTED) {
// Need to refresh the selected view
this.showSelected();
Expand Down Expand Up @@ -881,14 +855,8 @@ class ContentPicker extends Component<Props, State> {
if (!item[FIELD_SHARED_LINK]) {
this.changeShareAccess(null, item);
} else {
const { selected } = this.state;
const { id, type } = item;
const cacheKey = this.api.getAPI(type).getCacheKey(id);
// if shared link already exists, update the collection in state
// if shared link already exists, update the current collection
this.updateItemInCollection(item);
if (item.selected && item !== selected[cacheKey]) {
Copy link
Copy Markdown
Contributor

@priyajeet priyajeet Nov 7, 2019

Choose a reason for hiding this comment

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

Actually removing this part may cause a problem, since the item object in question has changed, it needs to be the one that is returned, so one will have to change the choose() function to return items from the updated collection and not whats inside selected.

Further simplification would be to make selected array just an array of IDs and Types, since thats what is really used everywhere to check if a row is selected or not. So that when choose button is pressed, then we build the returned array with latest item objects.

Copy link
Copy Markdown
Contributor

@priyajeet priyajeet Nov 7, 2019

Choose a reason for hiding this comment

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

Alternatively, updateItemInCollection() function can be changed to also update selected array with the updated item.

this.select(item, { forceSharedLink: false });
}
}
};

Expand Down Expand Up @@ -918,9 +886,6 @@ class ContentPicker extends Component<Props, State> {

this.api.getAPI(type).share(item, access, (updatedItem: BoxItem) => {
this.updateItemInCollection(updatedItem);
if (item.selected) {
this.select(updatedItem, { forceSharedLink: false });
}
});
};

Expand All @@ -931,17 +896,45 @@ class ContentPicker extends Component<Props, State> {
* @returns {void}
*/
updateItemInCollection = (item: BoxItem) => {
const { currentCollection } = this.state;
const { items = [] } = currentCollection;
const { currentCollection, selected: selectedItems = [] } = this.state;
const { items: collectionItems = [] } = currentCollection;
const newState = {
currentCollection: {
...currentCollection,
items: items.map(collectionItem => (collectionItem.id === item.id ? item : collectionItem)),
items: this.addUpdatedItemToCollection(item, collectionItems),
},
selected: this.addUpdatedItemToCollection(item, selectedItems),
};
this.setState(newState);
};

/**
* Inserts a given item into a collection of box items
* if a matching item is found in the collection.
*
* @private
* @param {BoxItem} item - the item that will be inserted into the collection
* @param {Array<BoxItem>} collection - the collection of items to be searched for a match
* @return {boolean}
*/

addUpdatedItemToCollection = (item: BoxItem, collection: Array<BoxItem>): Array<BoxItem> => {
return collection.map(collectionItem => (this.itemsMatch(collectionItem, item) ? item : collectionItem));
};

/**
* Determines if two items have the same id and type
*
* @private
* @param {BoxItem} item1 - the first item to compare
* @param {BoxItem} item2 - the second item to compare
* @return {boolean}
*/

itemsMatch = (item1: BoxItem, item2: BoxItem): boolean => {
return item1.id === item2.id && item1.type === item2.type;
};

/**
* Chages the sort by and sort direction
*
Expand Down Expand Up @@ -1152,7 +1145,7 @@ class ContentPicker extends Component<Props, State> {
}: State = this.state;
const { id, offset, permissions, totalCount }: Collection = currentCollection;
const { can_upload }: BoxItemPermission = permissions || {};
const selectedCount: number = Object.keys(selected).length;
const { length: selectedCount } = selected;
const isSingleSelect = maxSelectable === 1;
const hasHitSelectionLimit: boolean = selectedCount === maxSelectable && !isSingleSelect;
const allowUpload: boolean = canUpload && !!can_upload;
Expand Down Expand Up @@ -1202,6 +1195,7 @@ class ContentPicker extends Component<Props, State> {
onItemClick={this.onItemClick}
onFocusChange={this.onFocusChange}
onShareAccessChange={this.changeShareAccess}
selected={selected}
/>
<Footer
selectedCount={selectedCount}
Expand Down
23 changes: 19 additions & 4 deletions src/elements/content-picker/ItemList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { isFocusableElement, focus } from '../../utils/dom';
import shareAccessCellRenderer from './shareAccessCellRenderer';
import selectionCellRenderer from './selectionCellRenderer';
import isRowSelectable from './cellRendererHelper';
import { isSelected as isRowSelected } from './itemSelectionHelper';
import { VIEW_SELECTED, FIELD_NAME, FIELD_ID, FIELD_SHARED_LINK, TYPE_FOLDER } from '../../constants';

import './ItemList.scss';
Expand All @@ -34,6 +35,7 @@ type Props = {
rootElement?: HTMLElement,
rootId: string,
selectableType: string,
selected: Array<BoxItem>,
tableRef: Function,
view: View,
};
Expand All @@ -54,6 +56,7 @@ const ItemList = ({
onShareAccessChange,
onFocusChange,
currentCollection,
selected,
tableRef,
}: Props) => {
const iconCell = iconCellRenderer();
Expand All @@ -64,13 +67,15 @@ const ItemList = ({
extensionsWhitelist,
hasHitSelectionLimit,
isSingleSelect,
selected,
);
const shareAccessCell = shareAccessCellRenderer(
onShareAccessChange,
canSetShareAccess,
selectableType,
extensionsWhitelist,
hasHitSelectionLimit,
selected,
);
const { id, items = [] }: Collection = currentCollection;
const rowCount: number = items.length;
Expand All @@ -80,10 +85,18 @@ const ItemList = ({
return '';
}

const { selected, type } = items[index];
const isSelectable = isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, items[index]);
const { type } = items[index];
const isSelected = isRowSelected(items[index], selected);
const isSelectable = isRowSelectable(
selectableType,
extensionsWhitelist,
hasHitSelectionLimit,
items[index],
isSelected,
);

return classNames(`bcp-item-row bcp-item-row-${index}`, {
'bcp-item-row-selected': selected && view !== VIEW_SELECTED,
'bcp-item-row-selected': isSelected && view !== VIEW_SELECTED,
'bcp-item-row-unselectable': type !== TYPE_FOLDER && !isSelectable, // folder row should never dim
});
};
Expand All @@ -97,9 +110,11 @@ const ItemList = ({
index: number,
rowData: BoxItem,
}) => {
const isSelected = isRowSelected(rowData, selected);

// If the click is happening on a clickable element on the item row, ignore row selection
if (
isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, rowData) &&
isRowSelectable(selectableType, extensionsWhitelist, hasHitSelectionLimit, rowData, isSelected) &&
!isFocusableElement(event.target)
) {
onItemSelect(rowData);
Expand Down
Loading