From da6103b444c976c25dc53b174c68802e7cf2dbeb Mon Sep 17 00:00:00 2001 From: Jackson Harper Date: Thu, 23 Jun 2022 16:09:25 -0700 Subject: [PATCH] Fix issues with merging highlights highlightLocations can't be a prop or they wont be updated properly when highlights are created and merged. This fixes issues with merging highlights. It changes how we scroll to an initial highlight in the case of deep links too. --- .../components/templates/article/Article.tsx | 10 +-- .../templates/article/ArticleContainer.tsx | 24 +----- .../templates/article/HighlightsLayer.tsx | 73 +++++++++++++------ 3 files changed, 57 insertions(+), 50 deletions(-) diff --git a/packages/web/components/templates/article/Article.tsx b/packages/web/components/templates/article/Article.tsx index 69064446a..ec426cf52 100644 --- a/packages/web/components/templates/article/Article.tsx +++ b/packages/web/components/templates/article/Article.tsx @@ -21,7 +21,6 @@ import { ArticleMutations } from '../../../lib/articleActions' export type ArticleProps = { articleId: string content: string - highlightReady: boolean initialAnchorIndex: number initialReadingProgress?: number highlightHref: MutableRefObject @@ -128,14 +127,17 @@ export function Article(props: ArticleProps): JSX.Element { if (typeof window === 'undefined') { return } - - if (props.highlightReady) { if (!shouldScrollToInitialPosition) { return } setShouldScrollToInitialPosition(false) + // If we are scrolling to a highlight, dont scroll to read position + if (props.highlightHref.current) { + return + } + if (props.initialReadingProgress && props.initialReadingProgress >= 98) { return } @@ -165,9 +167,7 @@ export function Article(props: ArticleProps): JSX.Element { const calculatedOffset = calculateOffset(anchorElement) window.document.documentElement.scroll(0, calculatedOffset - 100) } - } }, [ - props.highlightReady, props.initialAnchorIndex, props.initialReadingProgress, shouldScrollToInitialPosition, diff --git a/packages/web/components/templates/article/ArticleContainer.tsx b/packages/web/components/templates/article/ArticleContainer.tsx index a79e415c6..6d0a09808 100644 --- a/packages/web/components/templates/article/ArticleContainer.tsx +++ b/packages/web/components/templates/article/ArticleContainer.tsx @@ -56,10 +56,6 @@ export function ArticleContainer(props: ArticleContainerProps): JSX.Element { const highlightHref = useRef( window.location.hash ? window.location.hash.split('#')[1] : null ) - const [highlightReady, setHighlightReady] = useState(false) - const [highlightLocations, setHighlightLocations] = useState< - HighlightLocation[] - >([]) const updateFontSize = async (newFontSize: number) => { if (fontSize !== newFontSize) { @@ -72,21 +68,6 @@ export function ArticleContainer(props: ArticleContainerProps): JSX.Element { updateFontSize(props.fontSize ?? 20) }, [props.fontSize]) - // Load the highlights - useEffect(() => { - const res: HighlightLocation[] = [] - props.article.highlights.forEach((highlight) => { - try { - const offset = makeHighlightStartEndOffset(highlight) - res.push(offset) - } catch (err) { - console.error(err) - } - }) - setHighlightLocations(res) - setHighlightReady(true) - }, [props.article.highlights, setHighlightLocations]) - // Listen for preference change events sent from host apps (ios, macos...) useEffect(() => { interface UpdateLineHeightEvent extends Event { @@ -275,10 +256,9 @@ export function ArticleContainer(props: ArticleContainerProps): JSX.Element { ) : null}
@@ -300,7 +280,7 @@ export function ArticleContainer(props: ArticleContainerProps): JSX.Element { setShowHighlightsModal: React.Dispatch> articleMutations: ArticleMutations - highlightLocations: HighlightLocation[] + } type HighlightModalAction = 'none' | 'addComment' | 'share' @@ -50,6 +51,9 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { const [highlightModalAction, setHighlightModalAction] = useState({ highlightModalAction: 'none' }) + const [highlightLocations, setHighlightLocations] = useState< + HighlightLocation[] + >([]) const focusedHighlightMousePos = useRef({ pageX: 0, pageY: 0 }) const [focusedHighlight, setFocusedHighlight] = useState< @@ -57,12 +61,36 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { >(undefined) const [selectionData, setSelectionData] = useSelection( - props.highlightLocations, + highlightLocations, false //noteModal.open, ) const canShareNative = useCanShareNative() + // Load the highlights + useEffect(() => { + const res: HighlightLocation[] = [] + highlights.forEach((highlight) => { + try { + const offset = makeHighlightStartEndOffset(highlight) + res.push(offset) + } catch (err) { + console.error(err) + } + }) + setHighlightLocations(res) + + // If we were given an initial highlight to scroll to we do + // that now that all the content has been injected into the + // page. + if (props.scrollToHighlight.current) { + const anchorElement = document.querySelector(`[omnivore-highlight-id="${props.scrollToHighlight.current}"]`) + if (anchorElement) { + anchorElement.scrollIntoView({ behavior: 'auto' }) + } + } + }, [highlights, setHighlightLocations]) + const removeHighlightCallback = useCallback( async (id?: string) => { const highlightId = id || focusedHighlight?.id @@ -73,7 +101,7 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { if (didDeleteHighlight) { removeHighlights( highlights.map(($0) => $0.id), - props.highlightLocations + highlightLocations ) setHighlights(highlights.filter(($0) => $0.id !== highlightId)) setFocusedHighlight(undefined) @@ -81,16 +109,16 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { console.error('Failed to delete highlight') } }, - [focusedHighlight, highlights, props.highlightLocations] + [focusedHighlight, highlights, highlightLocations] ) const updateHighlightsCallback = useCallback( (highlight: Highlight) => { - removeHighlights([highlight.id], props.highlightLocations) + removeHighlights([highlight.id], highlightLocations) const keptHighlights = highlights.filter(($0) => $0.id !== highlight.id) setHighlights([...keptHighlights, highlight]) }, - [highlights, props.highlightLocations] + [highlights, highlightLocations] ) const handleNativeShare = useCallback( @@ -143,7 +171,7 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { selection: selection, articleId: props.articleId, existingHighlights: highlights, - highlightStartEndOffsets: props.highlightLocations, + highlightStartEndOffsets: highlightLocations, annotation: note, }, props.articleMutations) @@ -198,21 +226,21 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { selectionData, setSelectionData, canShareNative, - props.highlightLocations, + highlightLocations, ] ) const scrollToHighlight = (id: string) => { const foundElement = document.querySelector(`[omnivore-highlight-id="${id}"]`) if(foundElement){ - foundElement.scrollIntoView({ - block: 'center', - behavior: 'smooth' - }) - window.location.hash = `#${id}` - props.setShowHighlightsModal(false) - } - } + foundElement.scrollIntoView({ + block: 'center', + behavior: 'smooth' + }) + window.location.hash = `#${id}` + props.setShowHighlightsModal(false) + } + } // Detect mouseclick on a highlight -- call `setFocusedHighlight` when highlight detected const handleClickHighlight = useCallback( @@ -257,7 +285,7 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { }) } else setFocusedHighlight(undefined) }, - [highlights, props.highlightLocations] + [highlights, highlightLocations] ) useEffect(() => { @@ -465,10 +493,9 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { if (props.showHighlightsModal) { return ( props.setShowHighlightsModal(false)} - scrollToHighlight={scrollToHighlight} - deleteHighlightAction={(highlightId: string) => { + highlights={highlights} + onOpenChange={() => props.setShowHighlightsModal(false)} + deleteHighlightAction={(highlightId: string) => { removeHighlightCallback(highlightId) }} /> @@ -476,4 +503,4 @@ export function HighlightsLayer(props: HighlightsLayerProps): JSX.Element { } return <> -} +} \ No newline at end of file