SUL23-438: Added location link to list of branches on the homepage #150

imonroe · 2024-06-21T17:55:47Z

READY FOR REVIEW

Summary

Added a map link to the homepage branches box.

Review By (Date)

When does this need to be reviewed by?

Criticality

How critical is this PR on a 1-10 scale? Also see Severity Assessment.
E.g., it affects one site, or every site and product?

Urgency

How urgent is this? (Normal, High)

Review Tasks

Setup tasks and/or behavior to test

Check out this branch
Navigate to the home page.
Select a branch from the dropdown in the top area of the page.
verify a map icon and link appears. Verify the link goes to a map to the chosen branch
Select a different library branch. Verify the link now goes to the newly-selected branch map.

Site Configuration Sync

Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Design is approved by @ user?
HTML validation: Is the markup using the appropriate semantic tags and passes validation? Or, QA request ticket created?
Cross-browser testing: Has been performed? Or, QA request ticket created?
Automated accessibility: Scans performed? Or, QA request ticket created?
Manual accessibility: Manually tested? Or, QA request ticket created?

Backend / Functional Validation

Code

Are the naming conventions following our standards?
Does the code have sufficient inline comments?
Is there anything in this code that would be hidden or hard to discover through the UI?
Are there any code smells?
Are tests provided? eg (unit, behat, or codeception)

Code security

Are all forms properly sanitized?
Any obvious security flaws or new areas for attack?

General

Is there anything included in this PR that is not related to the problem it is trying to solve?
Is the approach to the problem appropriate?

Affected Projects or Products

Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

JIRA ticket(s)
Other PRs
Any other contextual information that might be helpful (e.g., description of a bug that this PR fixes, new functionality that it adds, etc.)
Anyone who should be notified? (@mention them here)

Resources

AMP Tool
Accessibility Manual Test Script
HTML Validator
Browserstack and link to Browserstack Credentials

vercel · 2024-06-21T17:55:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
su-library	✅ Ready (Inspect)	Visit Preview	Jun 26, 2024 4:09pm

imonroe · 2024-06-21T18:01:12Z

Preview branch on Vercel is here: https://su-library-7f8up786g-stanford-libraries.vercel.app/

pookmish · 2024-06-21T19:03:37Z

src/components/node/stanford-page/home-page/home-page-banner.tsx

+        suLibraryHours: library.suLibraryHours,
+        suLibraryContactImg: library.suLibraryContactImg as MediaImage,
+        suLibraryBanner: library.suLibraryBanner as MediaImage,
+        map: library.suLibraryMapLink?.url ?? undefined


You don't need the ?? undefined

It threw a typescript error without it. Because of the Maybe I think.

Yeah. If you use map?: Maybe<string> it should fix that.

pookmish · 2024-06-21T19:05:27Z

src/components/node/stanford-page/home-page/today-hours.tsx

@@ -80,6 +83,9 @@ const LibrariesTodayHours = ({libraries, ...props}: { libraries: HoursProps["lib
              {library?.suLibraryHours &&
                <TodayLibraryHours branchId={library.suLibraryHours}/>
              }
+              {library?.map &&
+                <span className="float-right w-auto inline"><Link href={library?.map} aria-label="Link to map"><MapPinIcon title="Map" width={25} className="mr-5 inline"/>Location</Link></span>


I feel like some sr-only descriptive text would be helpful here. like <span className="sr-only">{library.title}</span> Location

I think I put this in the right place...

pookmish · 2024-06-24T18:18:34Z

src/components/node/stanford-page/home-page/today-hours.tsx

@@ -80,6 +84,9 @@ const LibrariesTodayHours = ({libraries, ...props}: { libraries: HoursProps["lib
              {library?.suLibraryHours &&
                <TodayLibraryHours branchId={library.suLibraryHours}/>
              }
+              {library?.map &&
+                <span className="float-right w-auto inline"><span className="sr-only">{library.title} Location</span><Link href={library?.map} aria-label="Link to map"><MapPinIcon title="Map" width={25} className="mr-5 inline"/>Location</Link></span>


the sr-only needs to be within the <Link> also you can remove the aria-label property.

Ok this is fixed.

pookmish · 2024-06-26T15:54:27Z

src/components/node/stanford-page/home-page/today-hours.tsx

@@ -80,6 +84,9 @@ const LibrariesTodayHours = ({libraries, ...props}: { libraries: HoursProps["lib
              {library?.suLibraryHours &&
                <TodayLibraryHours branchId={library.suLibraryHours}/>
              }
+              {library?.map &&
+                <span className="float-right w-auto inline"><Link href={library?.map} ><span className="sr-only">{library.title} Location</span><MapPinIcon title="Map" width={25} className="mr-5 inline"/>Location</Link></span>


Now you have 2 "location" strings. either wrap the second in aria-hidden or remove the first one. Ensure there is a space though. Sometimes you have to use &nbps; to force a space at the end of tag.

I removed the first one since the second is visible, and included the &nbps; for safety. Also got that git hook for prettier to do its thing correctly.

pookmish

Thanks.

SUL23-438: Added location link to list of branches on the homepage

23dc9a1

github-actions bot added the size/xs label Jun 21, 2024

imonroe requested a review from pookmish June 21, 2024 17:56

imonroe assigned pookmish Jun 21, 2024

imonroe requested a review from jenbreese June 21, 2024 17:56

pookmish requested changes Jun 21, 2024

View reviewed changes

Fixup from feedback

d3d9f0a

vercel bot deployed to Preview June 24, 2024 17:14 View deployment

pookmish reviewed Jun 24, 2024

View reviewed changes

fixup from feedback

702251d

vercel bot deployed to Preview June 26, 2024 15:52 View deployment

pookmish requested changes Jun 26, 2024

View reviewed changes

Removed redundant location label

7f2f60c

vercel bot deployed to Preview June 26, 2024 16:05 View deployment

fixup typo

4a0fe83

github-actions bot added size/s and removed size/xs labels Jun 26, 2024

vercel bot deployed to Preview June 26, 2024 16:09 View deployment

pookmish approved these changes Jun 26, 2024

View reviewed changes

pookmish merged commit 36d6f11 into 1.x Jun 26, 2024
2 of 4 checks passed

pookmish deleted the SUL23-438 branch June 26, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SUL23-438: Added location link to list of branches on the homepage #150

SUL23-438: Added location link to list of branches on the homepage #150

imonroe commented Jun 21, 2024

vercel bot commented Jun 21, 2024 •

edited

Loading

imonroe commented Jun 21, 2024

pookmish Jun 21, 2024

imonroe Jun 21, 2024

pookmish Jun 21, 2024 •

edited

Loading

pookmish Jun 21, 2024

imonroe Jun 24, 2024

pookmish Jun 24, 2024

imonroe Jun 26, 2024

pookmish Jun 26, 2024

imonroe Jun 26, 2024

pookmish left a comment

SUL23-438: Added location link to list of branches on the homepage #150

SUL23-438: Added location link to list of branches on the homepage #150

Conversation

imonroe commented Jun 21, 2024

READY FOR REVIEW

Summary

Review By (Date)

Criticality

Urgency

Review Tasks

Setup tasks and/or behavior to test

Site Configuration Sync

Front End Validation

Backend / Functional Validation

Code

Code security

General

Affected Projects or Products

Associated Issues and/or People

Resources

vercel bot commented Jun 21, 2024 • edited Loading

imonroe commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pookmish Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pookmish left a comment

Choose a reason for hiding this comment

vercel bot commented Jun 21, 2024 •

edited

Loading

pookmish Jun 21, 2024 •

edited

Loading