Skip to content
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

Use IntersectionObserver to detect current page #3682

Closed
wants to merge 2 commits into from
Closed

Use IntersectionObserver to detect current page #3682

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8abdd58
Add IntersectionObserver to BrewPage
G-Ambatte Aug 29, 2024
335f30f
Round down the current page calculation
G-Ambatte Aug 29, 2024
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
27 changes: 24 additions & 3 deletions client/homebrew/brewRenderer/brewRenderer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const RenderWarnings = require('homebrewery/renderWarnings/renderWarnings.jsx');
const NotificationPopup = require('./notificationPopup/notificationPopup.jsx');
const Frame = require('react-frame-component').default;
const dedent = require('dedent-tabs').default;
const { printCurrentBrew } = require('../../../shared/helpers.js');
const { printCurrentBrew, updatePageArray, getPageNumber } = require('../../../shared/helpers.js');

const DOMPurify = require('dompurify');
const purifyConfig = { FORCE_BODY: true, SANITIZE_DOM: false };
Expand All @@ -35,8 +35,28 @@ const BrewPage = (props)=>{
index : 0,
...props
};

const pageRef = useRef();

const rootMargin = '-50% 0px';
Copy link
Member

@calculuschild calculuschild Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain the behavior of how rootMargin works here? Any difference between this and threshold instead?

Maybe just add a brief comment explaining how this boundary determines the current page.


useEffect(()=>{
const observer = new IntersectionObserver(
([entry])=>{
const pageNo = parseInt(entry.target.getAttribute('id').slice(1));
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can just use the component's index prop directly instead of reading it from the DOM target?

updatePageArray(entry.isIntersecting, pageNo);
},
{ rootMargin }
);
observer.observe(pageRef.current);
return ()=>{
if(pageRef.current == null) return;
observer.unobserve(pageRef.current);
};
}, [pageRef.current, rootMargin]);
Copy link
Member

@calculuschild calculuschild Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Neither of these values trigger a redraw when changed, so they won't have any effect here in the useEffect dependency array. The effect will just fire once when the page component is mounted and then never again. In that case, this is equivalent to just an empty array [].


const cleanText = props.contents; //DOMPurify.sanitize(props.contents, purifyConfig);
return <div className={props.className} id={`p${props.index + 1}`} >
return <div className={props.className} id={`p${props.index + 1}`} ref={pageRef} >
<div className='columnWrapper' dangerouslySetInnerHTML={{ __html: cleanText }} />
</div>;
};
Expand Down Expand Up @@ -112,7 +132,8 @@ const BrewRenderer = (props)=>{
{props.renderer}
</div>
<div>
{state.viewablePageNumber + 1} / {rawPages.length}
{/* {state.viewablePageNumber + 1} / {rawPages.length} */}
{getPageNumber()} / {rawPages.length}
</div>
</div>;
};
Expand Down
34 changes: 34 additions & 0 deletions shared/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,42 @@ const fetchThemeBundle = async (obj, renderer, theme)=>{
}));
};

let pageArray = [];
Copy link
Member

@calculuschild calculuschild Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What is the purpose of the pageArray instead of simply setting the current page directly from the page that triggered the intersectionObserver event?

Seems we could get away with a simple getPage and setPage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thinking was that - particularly with the new BrewRenderer toolbar - that there can be multiple pages visible in the view port, so an array of page numbers is more appropriate than a single value.

let currentPage = 1;

const updatePageArray = (add, pageNo)=>{
// Remove page # from array
if(!add){
// If page # not in array, exit
if(!pageArray.includes(pageNo)) return;
// Update array to exclude page #
pageArray = pageArray.filter((el)=>{return el != pageNo;});
return;
}
// Add page # to array
// If page # already in array, exit
if(pageArray.includes(pageNo)) return;
// Add page # to array
pageArray.push(pageNo);
return;
};

const getPageNumber = ()=>{
if(pageArray.length == 0) return currentPage;
currentPage = Math.floor(pageArray.reduce((p, c)=>{p+c;}) / pageArray.length);
return currentPage;
};

const getVisiblePageRange = ()=>{
if(pageArray.length <= 3) return pageArray.join(', ');
return `${pageArray[0]} - ${pageArray.slice(-1)[0]}`;
};

module.exports = {
splitTextStyleAndMetadata,
printCurrentBrew,
fetchThemeBundle,
updatePageArray,
getPageNumber,
getVisiblePageRange
};