Skip to content

Commit

Permalink
Merge pull request #22 from art-institute-of-chicago/combined-snags-1…
Browse files Browse the repository at this point in the history
…86763291

Combined snags
  • Loading branch information
jonw-cogapp authored Jan 10, 2024
2 parents 9b4dde7 + 273a448 commit 22060da
Show file tree
Hide file tree
Showing 16 changed files with 518 additions and 504 deletions.
820 changes: 411 additions & 409 deletions dist/CustomTourBuilder.js

Large diffs are not rendered by default.

10 changes: 2 additions & 8 deletions src/CustomTourBuilder.cy.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ describe("<CustomTourBuilder />", () => {
cy.get("#aic-ct-search-results__loading").should("have.text", "Loading...");
cy.get("#aic-ct-search-results__items").should("exist");
cy.get("#aic-ct-search-results__items li").should("have.length", 10);
cy.get("#aic-ct-search-result-count").should("have.text", "10");
});

it("Can perform a search on a theme and show results", () => {
Expand All @@ -98,7 +97,6 @@ describe("<CustomTourBuilder />", () => {
cy.get("#aic-ct-search-results__loading").should("have.text", "Loading...");
cy.get("#aic-ct-search-results__items").should("exist");
cy.get("#aic-ct-search-results__items li").should("have.length", 10);
cy.get("#aic-ct-search-result-count").should("have.text", "10");
cy.get("#aic-ct-theme-toggle-0").click();
cy.get("#aic-ct-search-results__items").should("not.exist");
});
Expand All @@ -117,9 +115,7 @@ describe("<CustomTourBuilder />", () => {
cy.get("#aic-ct-search-item-75644 button").click();
cy.get("#aic-ct-preview__action-button-75644").click();
cy.get("#aic-ct-item-count").should("have.text", "3");
cy.get("#aic-ct-header__next-button")
.should("have.text", "Preview")
.click();
cy.get("#aic-ct-header__next-button").should("have.text", "Next").click();
cy.get("#aic-ct-tour-item-59426").should("exist");
cy.get("#aic-ct-tour-item-243872").should("exist");
cy.get("#aic-ct-tour-item-75644").should("exist");
Expand All @@ -145,9 +141,7 @@ describe("<CustomTourBuilder />", () => {
cy.get("#aic-ct-search-item-75644 button").click();
cy.get("#aic-ct-preview__action-button-75644").click();
cy.get("#aic-ct-item-count").should("have.text", "3");
cy.get("#aic-ct-header__next-button")
.should("have.text", "Preview")
.click();
cy.get("#aic-ct-header__next-button").should("have.text", "Next").click();
cy.get("#aic-ct-tour-item-59426").should("exist");
cy.get("#aic-ct-tour-item-243872").should("exist");
cy.get("#aic-ct-tour-item-75644").should("exist");
Expand Down
14 changes: 7 additions & 7 deletions src/components/navigation/Header.cy.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,20 @@ describe("<Header />", () => {
<Header />
</AppProvider>,
);
cy.get("#aic-ct-header__back-button").should("have.text", "Exit");
cy.get("#aic-ct-header__next-button").should("have.text", "Preview");
cy.get("#aic-ct-header__back-button").should("have.text", "Back");
cy.get("#aic-ct-header__next-button").should("have.text", "Next");
cy.get("#aic-ct-header__next-button").click();
cy.get("#aic-ct-header__back-button").should("have.text", "Search");
cy.get("#aic-ct-header__back-button").should("have.text", "Back");
cy.get("#aic-ct-header__next-button").should("have.text", "Finish");
cy.get("#aic-ct-header__next-button").click();
cy.get("#aic-ct-header__back-button").should("have.text", "Preview");
cy.get("#aic-ct-header__back-button").should("have.text", "Back");
cy.get("#aic-ct-header__next-button").should("have.text", "Finish");
cy.get("#aic-ct-header__back-button").click();
cy.get("#aic-ct-header__back-button").should("have.text", "Search");
cy.get("#aic-ct-header__back-button").should("have.text", "Back");
cy.get("#aic-ct-header__next-button").should("have.text", "Finish");
cy.get("#aic-ct-header__back-button").click();
cy.get("#aic-ct-header__back-button").should("have.text", "Exit");
cy.get("#aic-ct-header__next-button").should("have.text", "Preview");
cy.get("#aic-ct-header__back-button").should("have.text", "Back");
cy.get("#aic-ct-header__next-button").should("have.text", "Next");
});
it("Can exit the builder", () => {
cy.mount(
Expand Down
6 changes: 2 additions & 4 deletions src/components/navigation/Header.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ function Header() {
<svg className="icon--arrow" aria-hidden="true">
<use xlinkHref="#icon--arrow"></use>
</svg>
{activeNavPage === 0 && "Exit"}
{activeNavPage === 1 && "Search"}
{activeNavPage === 2 && "Preview"}
Back
</button>
<button
ref={headerNextButtonRef}
Expand All @@ -64,7 +62,7 @@ function Header() {
}
}}
>
{activeNavPage === 0 && "Preview"}
{activeNavPage === 0 && "Next"}
{activeNavPage > 0 && "Finish"}
<svg className="icon--arrow" aria-hidden="true">
<use xlinkHref="#icon--arrow"></use>
Expand Down
2 changes: 2 additions & 0 deletions src/components/search/SearchBar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ function SearchBar() {
setSearchQuery("");
setSearchResultItems(null);
setActiveTheme(null);
// Apply results for empty keyword search
fetchData(createSearchUrl({ keywords: "" }));
searchButtonRef.current.focus();
}}
>
Expand Down
31 changes: 18 additions & 13 deletions src/components/search/SearchPreview.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import classNames from "classnames";
function SearchPreview() {
const { searchPreviewId, searchResultItems, searchPreviewRef } =
useContext(SearchContext);
const { iiifBaseUrl, tourItems, tourItemsDispatch } = useContext(AppContext);
const { iiifBaseUrl, tourItems, tourItemsDispatch, limits } =
useContext(AppContext);
const [inTour, setInTour] = useState(false);
const [previewData, setPreviewData] = useState(null);

Expand Down Expand Up @@ -103,7 +104,7 @@ function SearchPreview() {
{/* If the item isn't in the tour and the limit isn't reached: show add */}
{/* Otherwise don't show a button */}
{/* Needs to be done in a way that doesn't remove this button and lose focus */}
{(tourItems.length < 6 || inTour) && (
{tourItems.length < 6 || inTour ? (
<button
id={`aic-ct-preview__action-button-${previewData.id}`}
className="btn f-buttons aic-ct-preview__action-button"
Expand All @@ -114,6 +115,12 @@ function SearchPreview() {
>
{inTour ? "Remove from your tour" : "Add to your tour"}
</button>
) : (
<p className="f-body">
You have already added {limits.items.max} artworks, the
maximum number allowed. Please remove one if you would like to
include this artwork.
</p>
)}
</div>

Expand All @@ -122,18 +129,16 @@ function SearchPreview() {
<h3 className="aic-ct-preview__description-title f-module-title-2">
Artwork description
</h3>
{/* It appears short_description lacks wrapping html or formatting,
{/* N.b. It appears short_description lacks wrapping html or formatting,
whereas full descriptions have this */}
{previewData.short_description ? (
<p className="f-body">{previewData.short_description}</p>
) : (
<div
className="f-body"
dangerouslySetInnerHTML={{
__html: previewData.description,
}}
></div>
)}
<div
className="f-body"
dangerouslySetInnerHTML={{
__html: previewData.short_description
? previewData.short_description
: previewData.description,
}}
></div>
<a
className="aic-ct-preview__learn-more f-link"
target="_blank"
Expand Down
5 changes: 4 additions & 1 deletion src/components/search/SearchResultItem.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ function SearchResultItem(props) {
searchPreviewRef.current.showModal();
// Reset the scroll position after the modal has opened
setTimeout(() => {
document.documentElement.classList.add("s-body-locked");
document.documentElement.classList.add(
"s-body-locked",
"s-body-locked--ct",
);
document.body.scrollTop = _scrollY;
}, 0);
};
Expand Down
1 change: 0 additions & 1 deletion src/components/search/SearchResults.cy.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,5 @@ describe("<SearchResults />", () => {
);
cy.get("#aic-ct-search-results__items").should("exist");
cy.get("#aic-ct-search-results__items li").should("have.length", 1);
cy.get("#aic-ct-search-result-count").should("have.text", "1");
});
});
65 changes: 46 additions & 19 deletions src/components/search/SearchResults.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,32 +17,68 @@ function SearchResults() {
const { scrollY } = useContext(AppContext);

// Store a reference to the event listener callback to remove it later
const handleClose = useCallback(() => {
setSearchPreviewId(null);
document.documentElement.scrollTop = scrollY;
document.documentElement.classList.remove("s-body-locked");
}, [setSearchPreviewId, scrollY]);
const handleClose = useCallback(
(e) => {
// Dual use for this function during the close event and the click event
if (
e.type === "close" ||
(searchPreviewRef?.current?.open &&
e.target === searchPreviewRef?.current)
) {
// Close the window if open -- does nothing on close event
searchPreviewRef.current.close();
setSearchPreviewId(null);

document.documentElement.scrollTop = scrollY;
document.documentElement.classList.remove(
"s-body-locked",
"s-body-locked--ct",
);
}
},
[setSearchPreviewId, scrollY, searchPreviewRef],
);

const handlePageUpdated = useCallback(() => {

Check warning on line 42 in src/components/search/SearchResults.jsx

View workflow job for this annotation

GitHub Actions / Build

React Hook useCallback does nothing when called with only one argument. Did you forget to pass an array of dependencies?
const evt = new Event("page:updated", { bubbles: true });
setTimeout(() => {
document.dispatchEvent(evt);
}, 0);
});

useEffect(() => {
const ref = searchPreviewRef.current;

if (ref) {
ref.addEventListener("close", handleClose);
ref.addEventListener("click", handleClose);
}
return () => {
if (ref) {
ref.removeEventListener("close", handleClose);
ref.removeEventListener("click", handleClose);
}
};
}, [searchPreviewRef, handleClose]);

// This does dispatch multiple times, but it doesn't seem to cause any issues
// Might consider debouncing this
useEffect(() => {
const evt = new Event("page:updated", { bubbles: true });
setTimeout(() => {
document.dispatchEvent(evt);
}, 0);
}, [searchResultItems]);
// Relayout pinboard when results change
handlePageUpdated();
}, [searchResultItems, handlePageUpdated]);

// Safari in particular (but potentially unnoticed in other browsers)
// Would sometimes fire page:updated, and not be picked up by the pinboard listener
// Possibly some kind of race condition?
// Safest way to have this always fire regardless is when page finishes loading.
// (in addition to when search results change)
useEffect(() => {
window.addEventListener("load", handlePageUpdated);
return () => {
window.removeEventListener("load", handlePageUpdated);
};
}, [handlePageUpdated]);

// Catch all for no results, error, and loading states
if (!searchResultItems && !searchFetching && !searchError) {
Expand Down Expand Up @@ -83,15 +119,6 @@ function SearchResults() {
{searchResultItems?.length > 0 && !searchFetching && !searchError && (
// Render the results if there are results
<>
<header className="aic-ct-section-header f-body">
<h2 className="f-module-title-2">Browse Artworks</h2>
<span
id="aic-ct-search-result-count"
className="aic-ct-item-count aic-ct-item-count--body"
>
{searchResultItems.length}
</span>
</header>
<p className="aic-ct-pre-result-text f-body">
These artworks are currently on view and available for your tour.
</p>
Expand Down
6 changes: 3 additions & 3 deletions src/components/search/ThemeToggle.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function ThemeToggle(props) {
activeTheme,
setActiveTheme,
} = useContext(SearchContext);
const { fetchData, resetState } = useFetch({
const { fetchData } = useFetch({
dataSubSelector: "data",
dataSetter: setSearchResultItems,
fetchingSetter: setSearchFetching,
Expand All @@ -31,8 +31,8 @@ function ThemeToggle(props) {
if (activeTheme === label) {
// Deselect the theme
setActiveTheme(null);
// Should remove results
resetState();
// Apply results for empty keyword search
fetchData(createSearchUrl({ keywords: "" }));
} else {
fetchData(createSearchUrl(searchParams));
// Clicking while active removes the theme
Expand Down
29 changes: 12 additions & 17 deletions src/components/search/Themes.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function Themes() {
label: "Essentials",
thumbnailId: "b272df73-a965-ac37-4172-be4e99483637",
searchParams: {
theme_title: "Essentials",
category_ids: ["PC-831"],
},
},
{
Expand All @@ -46,22 +46,17 @@ function Themes() {
];

return (
<>
<header className="aic-ct-section-header">
<h2 className="f-module-title-2">Browse by theme</h2>
</header>
<ul id="aic-ct-themes" className="aic-ct-themes">
{themes.map((theme, index) => (
<ThemeToggle
key={theme.label}
id={index}
label={theme.label}
thumbnailId={theme.thumbnailId}
searchParams={theme.searchParams}
/>
))}
</ul>
</>
<ul id="aic-ct-themes" className="aic-ct-themes">
{themes.map((theme, index) => (
<ThemeToggle
key={theme.label}
id={index}
label={theme.label}
thumbnailId={theme.thumbnailId}
searchParams={theme.searchParams}
/>
))}
</ul>
);
}

Expand Down
8 changes: 3 additions & 5 deletions src/components/submission/Submission.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,9 @@ function Submission() {
id="aic-ct-validation__save"
className="aic-ct-validation__save aic-ct-validation__content"
>
<h1 className="f-headline">
Are you sure you want to submit your tour?
</h1>
<h1 className="f-headline">Are you ready to finish your tour?</h1>
<p className="f-body">
You won&apos;t be able to edit it once this has been done.
You won&apos;t be able to edit it once you save.
<br />
Your tour will be automatically emailed to you when finished.
</p>
Expand All @@ -210,7 +208,7 @@ function Submission() {
onClick={handleSave}
disabled={isSaving}
>
Yes, Save my tour
Yes, save my tour
</button>
<button
className="btn btn--secondary f-buttons"
Expand Down
4 changes: 2 additions & 2 deletions src/components/tour/TourItem.cy.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ describe("<TourItem />", () => {
it("Renders", () => {
cy.intercept(
"GET",
"https://artic.edu/iiif/2/test_image_id/full/!96,128/0/default.jpg",
"https://artic.edu/iiif/2/test_image_id/square/!128,128/0/default.jpg",
{
fixture: `../../cypress/fixtures/images/image_${
Math.floor(Math.random() * 10) + 1
Expand All @@ -31,7 +31,7 @@ describe("<TourItem />", () => {
.should(
"have.attr",
"src",
"https://artic.edu/iiif/2/test_image_id/full/!96,128/0/default.jpg",
"https://artic.edu/iiif/2/test_image_id/square/!128,128/0/default.jpg",
)
.should("have.attr", "alt", "Test image alt text");
});
Expand Down
Loading

0 comments on commit 22060da

Please sign in to comment.