Skip to content

Commit

Permalink
Fix item not always scrolled into view #253 #193
Browse files Browse the repository at this point in the history
  • Loading branch information
tnajdek committed Aug 16, 2024
1 parent a3e6469 commit d2821f3
Show file tree
Hide file tree
Showing 12 changed files with 23,158 additions and 27,224 deletions.
6 changes: 3 additions & 3 deletions src/js/actions/items-export.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const exportItems = (itemKeys, libraryKey, format) => {
const response = await api(config.apiKey, config.apiConfig)
.library(libraryKey)
.items()
.get({ itemKey: itemKeys.join(','), includeTrashed: true, format });
.get({ itemKey: itemKeys.join(','), includeTrashed: 1, format });

const exportData = await response.response.blob();

Expand Down Expand Up @@ -119,7 +119,7 @@ const citeItems = (itemKeys, libraryKey, style = 'chicago-note-bibliography', lo
.get({
itemKey: itemKeys.join(','),
include: 'citation',
includeTrashed: true,
includeTrashed: 1,
style,
locale
});
Expand Down Expand Up @@ -165,7 +165,7 @@ const bibliographyFromItems = (itemKeys, libraryKey, style = 'chicago-note-bibli
.get({
itemKey: itemKeys.join(','),
format: 'bib',
includeTrashed: true,
includeTrashed: 1,
style,
locale,
});
Expand Down
79 changes: 76 additions & 3 deletions src/js/actions/items-read.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { SORT_ITEMS, REQUEST_ATTACHMENT_URL, RECEIVE_ATTACHMENT_URL, ERROR_ATTACHMENT_URL } from '../constants/actions';
import api from 'zotero-api-client';
import { escapeBooleanSearches, extractItems, getApiForItems } from '../common/actions';
import { getItemKeysPath } from '../common/state';
import { get, getAbortController, mapRelationsToItemKeys } from '../utils';
import columnProperties from '../constants/column-properties';
import { connectionIssues, requestTracker, requestWithBackoff } from '.';
Expand Down Expand Up @@ -86,9 +87,13 @@ const fetchItemsByKeys = (itemKeys, queryOptions, overrides) => {
// @NOTE: same as fetch items but limited to one item and ignored when considering membership
// (collection, trash etc.).
const fetchItemDetails = (itemKey, queryOptions, overrides) => {
return fetchItems(
'FETCH_ITEM_DETAILS', {}, { ...queryOptions, itemKey }, overrides
);
return async (dispatch, getState) => {
const state = getState();
const includeTrashed = state.current.isTrash ? 1 : 0;
return dispatch(fetchItems(
'FETCH_ITEM_DETAILS', {}, { ...queryOptions, includeTrashed, itemKey }, overrides
));
};
}

const fetchAllItemsSince = (version, queryOptions, overrides) => {
Expand Down Expand Up @@ -313,6 +318,73 @@ const fetchAllChildItems = (itemKey, queryOptions = {}, overrides) => {
}
}

const findRowIndexInSource = () => {
return async (dispatch, getState) => {
const state = getState();
const itemKey = state.current.itemKeys?.[0];

if(itemKey === null || typeof itemKey === 'undefined') {
return 0;
}

const libraryKey = state.current.libraryKey;
const config = state.config;
const { collectionKey, isTrash, isMyPublications, itemsSource, search: q, qmode,
tags: tag = [] } = state.current;
const { field: sortBy, sort: sortDirection } = state.preferences.columns.find(
column => 'sort' in column) || { field: 'title', sort: 'asc' };

const direction = sortDirection.toLowerCase();
const sort = (sortBy in columnProperties && columnProperties[sortBy].sortKey) || 'title';
const sortAndDirection = { sort, direction };

let type = 'TOP_ITEMS';
let queryConfig = {};
let queryOptions = { ...sortAndDirection };
let path = getItemKeysPath({ itemsSource, libraryKey, collectionKey });
let keys = get(state, [...path, 'keys'], []);

switch (itemsSource) {
case 'query':
type = 'ITEMS_BY_QUERY';
queryConfig = { collectionKey, isMyPublications, isTrash, };
queryOptions = { q, tag, qmode, ...sortAndDirection };
break;
case 'top':
type = 'TOP_ITEMS';
break;
case 'trash':
type = 'TRASH_ITEMS';
break;
case 'publications':
type = 'PUBLICATIONS_ITEMS';
break;
case 'collection':
type = 'ITEMS_IN_COLLECTION';
queryConfig = { collectionKey };
break;
}

let itemIndexLocal = keys.indexOf(itemKey);

if (itemIndexLocal !== -1) {
return itemIndexLocal;
}

try {
const api = getApiForItems({ config, libraryKey }, type, queryConfig);
const response = await api.get({ ...escapeBooleanSearches(queryOptions, 'tag'), format: 'keys' });
const keys = (await response.getData().text()).split("\n");
if(keys.indexOf(itemKey) !== -1) {
return keys.indexOf(itemKey);
}
return 0;
} catch (error) {
return 0;
}
}
}

export {
fetchAllChildItems,
fetchAllItemsSince,
Expand All @@ -326,6 +398,7 @@ export {
fetchSource,
fetchTopItems,
fetchTrashItems,
findRowIndexInSource,
getAttachmentUrl,
sortItems,
};
19 changes: 18 additions & 1 deletion src/js/actions/navigate.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { makePath } from '../common/navigation';
import { getItemKeysPath } from '../common/state';
import { LOCATION_CHANGE, push } from 'connected-react-router';
import { TRIGGER_VIEWPORT_CHANGE } from '../constants/actions';
import { clamp, get } from '../utils';

const pushEmbedded = (path) => ({
Expand Down Expand Up @@ -32,7 +33,18 @@ const currentToPath = current => ({
location: current.location,
});

const navigate = (path, isAbsolute = false) => {

/**
* Navigates to a specified path.
*
* @param {string} path - The path to navigate to.
* @param {boolean} [isAbsolute=false] - Indicates whether the path is absolute or relative to the current path.
* @param {boolean} [viewportChange=false] - Indicates whether a viewport change should be triggered after navigation.
* This, for example, triggers table list to query for all item keys so that it can figure out where in the list target item is.
* @returns {Function} - The async action function.
*/
const navigate = (path, isAbsolute = false, viewportChange = false) => {
// console.trace('navigate', { path, isAbsolute, viewportChange });
return async (dispatch, getState) => {
const { config, current } = getState();
const isEmbedded = config.isEmbedded;
Expand All @@ -48,6 +60,11 @@ const navigate = (path, isAbsolute = false) => {
const configuredPath = makePath(config, updatedPath);
dispatch(pushFn(configuredPath));
}
if (viewportChange) {
dispatch({
type: TRIGGER_VIEWPORT_CHANGE,
});
}
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/js/component/item-details/related.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const RelatedItem = memo(props => {
dispatch(navigate({
library: libraryKey,
items: relatedItemKey
}, true));
}, true, true));
}, [dispatch, libraryKey]);

const handleDelete = useCallback(ev => {
Expand Down
84 changes: 46 additions & 38 deletions src/js/component/item/items/list.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { memo, useCallback, useEffect, useRef } from 'react';
import { memo, useCallback, useEffect, useRef, useState } from 'react';
import { useDispatch, useSelector, shallowEqual } from 'react-redux';
import PropTypes from 'prop-types';
import cx from 'classnames';
Expand All @@ -12,6 +12,7 @@ import ListRow from './list-row';
import { abortAllRequests, connectionIssues, fetchSource } from '../../../actions';
import { useSourceData } from '../../../hooks';
import { get, getRequestTypeFromItemsSource } from '../../../utils';
import ScrollEffectComponent, { SCROLL_BUFFER } from './scroll-effect';

const ROWHEIGHT = 61;

Expand All @@ -33,6 +34,7 @@ const ItemsList = memo(props => {
const requestType = getRequestTypeFromItemsSource(itemsSource);
const errorCount = useSelector(state => get(state, ['traffic', requestType, 'errorCount'], 0));
const prevErrorCount = usePrevious(errorCount);
const [scrollToRow, setScrollToRow] = useState(null);

//@NOTE: On mobiles (single-column) we have a dedicated search mode where. To prevent visual glitches
// where current items overlap empty search prompt we need the following hack. See #230
Expand All @@ -54,11 +56,13 @@ const ItemsList = memo(props => {
}, [dispatch]);

useEffect(() => {
if(!hasChecked && !isFetching) {
dispatch(fetchSource(0, 50));
lastRequest.current = { startIndex: 0, stopIndex: 50 };
if (scrollToRow !== null && !hasChecked && !isFetching) {
let startIndex = Math.max(scrollToRow - 20, 0);
let stopIndex = scrollToRow + 50;
dispatch(fetchSource(startIndex, stopIndex));
lastRequest.current = { startIndex, stopIndex };
}
}, [dispatch, isFetching, hasChecked]);
}, [dispatch, isFetching, hasChecked, scrollToRow]);

useEffect(() => {
if(errorCount > 0 && errorCount > prevErrorCount) {
Expand All @@ -75,7 +79,7 @@ const ItemsList = memo(props => {
}, [dispatch, errorCount, prevErrorCount]);

useEffect(() => {
if(prevSortBy === sortBy && prevSortDirection === sortDirection) {
if ((typeof prevSortBy === 'undefined' && typeof prevSortDirection === 'undefined') || (prevSortBy === sortBy && prevSortDirection === sortDirection)) {
return;
}

Expand Down Expand Up @@ -104,40 +108,44 @@ const ItemsList = memo(props => {

return (
<div className="items-list-wrap">
<AutoSizer>
{({ height, width }) => (
<InfiniteLoader
ref={ loader }
listRef={ listRef }
isItemLoaded={ handleIsItemLoaded }
itemCount={ hasChecked && !isSearchModeHack ? totalResults : 0 }
loadMoreItems={ handleLoadMore }
>
{({ onItemsRendered, ref }) => (
<div
aria-label="items"
className={ cx('items-list', {
'editing': isSelectMode,
}) }
role={ isSelectMode ? 'listbox' : 'list' }
aria-rowcount={ totalResults }
>
<List
height={ height }
itemCount={ hasChecked && !isSearchModeHack ? totalResults : 0 }
itemData={ { keys } }
itemSize={ ROWHEIGHT }
onItemsRendered={ onItemsRendered }
ref={ r => { ref(r); listRef.current = r; } }
width={ width }
<ScrollEffectComponent listRef={listRef} setScrollToRow={setScrollToRow} />
{ hasChecked && (
<AutoSizer>
{({ height, width }) => (
<InfiniteLoader
ref={ loader }
listRef={ listRef }
isItemLoaded={ handleIsItemLoaded }
itemCount={ hasChecked && !isSearchModeHack ? totalResults : 0 }
loadMoreItems={ handleLoadMore }
>
{({ onItemsRendered, ref }) => (
<div
aria-label="items"
className={ cx('items-list', {
'editing': isSelectMode,
}) }
role={ isSelectMode ? 'listbox' : 'list' }
aria-rowcount={ totalResults }
>
{ ListRow }
</List>
</div>
)}
</InfiniteLoader>
<List
initialScrollOffset={Math.max(scrollToRow - SCROLL_BUFFER, 0) * ROWHEIGHT}
height={ height }
itemCount={ hasChecked && !isSearchModeHack ? totalResults : 0 }
itemData={ { keys } }
itemSize={ ROWHEIGHT }
onItemsRendered={ onItemsRendered }
ref={ r => { ref(r); listRef.current = r; } }
width={ width }
>
{ ListRow }
</List>
</div>
)}
</InfiniteLoader>
)}
</AutoSizer>
)}
</AutoSizer>
{ !hasChecked && !isSearchModeHack && <Spinner className="large" /> }
{ hasChecked && totalResults === 0 && (
<div className="item-list-empty">
Expand Down
70 changes: 70 additions & 0 deletions src/js/component/item/items/scroll-effect.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import { memo, useCallback, useEffect, } from 'react';
import { useDispatch, useSelector, shallowEqual } from 'react-redux';
import { usePrevious } from 'web-common/hooks';
import PropTypes from 'prop-types';

import { findRowIndexInSource } from '../../../actions';
import { useSourceData } from '../../../hooks';

export const SCROLL_BUFFER = 5;

const ScrollEffectComponent = memo(({ listRef, setScrollToRow }) => {
const dispatch = useDispatch();
const selectedItemKeys = useSelector(state => state.current.itemKeys);
const { keys } = useSourceData();
const previousSelectedItemKeys = usePrevious(selectedItemKeys);
const isItemsTableFocused = useSelector(state => state.current.isItemsTableFocused);

const viewportCount = useSelector(state => state.current.viewportCount);
const prevViewportCount = usePrevious(viewportCount);

const findItemsPositionFromRemote = useCallback(async (skipScroll = false) => {
// findRowIndexInSource won't run a request if no item is specified in the URL or if
// specified item is already loaded
let nextScrollToRow = await dispatch(findRowIndexInSource());

// Set scrollToRow. The Table component will now run the first fetch for items around the
// target. Once it does, it will set hasChecked to true.
setScrollToRow(nextScrollToRow);

if (!skipScroll) {
let bufferedScrollToRow = Math.max(nextScrollToRow - SCROLL_BUFFER, 0);
listRef.current?.scrollToItem?.(bufferedScrollToRow, 'start');
}
}, [dispatch, listRef, setScrollToRow])

// Initial load: findItemsPositionFromRemote but skip scrolling. Once scrollToRow is set, the Table
// component will mount the list with initialScrollOffset set to the correct value.
useEffect(() => {
findItemsPositionFromRemote(true);
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// Triggers when a "viewport change" navigation happens, e.g., a reference from a note or a related item.
useEffect(() => {
if (typeof prevViewportCount !== 'undefined' && viewportCount !== prevViewportCount) {
setScrollToRow(null);
findItemsPositionFromRemote();
}
}, [viewportCount, prevViewportCount, findItemsPositionFromRemote, setScrollToRow]);

// Scroll to the selected item, e.g., when using keyboard navigation.
useEffect(() => {
if (listRef.current && keys && selectedItemKeys.length > 0 && !shallowEqual(selectedItemKeys, previousSelectedItemKeys)) {
const itemKey = selectedItemKeys[selectedItemKeys.length - 1];
const itemKeyIndex = keys.findIndex(k => k === itemKey);
if (itemKeyIndex !== -1) {
listRef.current.scrollToItem(itemKeyIndex, 'smart');
}
}
}, [selectedItemKeys, isItemsTableFocused, keys, listRef, previousSelectedItemKeys]);
return null;
});

ScrollEffectComponent.propTypes = {
listRef: PropTypes.object.isRequired,
setScrollToRow: PropTypes.func.isRequired,
};

ScrollEffectComponent.displayName = "TableScroll";

export default ScrollEffectComponent;
Loading

0 comments on commit d2821f3

Please sign in to comment.