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.

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

Website 38 investigate adding the ability to navigate via calendar in hours view #319

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

SalazarJosh
Copy link
Collaborator

@SalazarJosh SalazarJosh commented Nov 18, 2024

Overview

Users can only navigate one week a time on the Hours View, which makes it tedious to look further ahead.

This PR adds a calendar component to the hours view (deploy URL). The calendar component provides a singular component to give the user the ability to navigate by weeks while also having a monthly calendar display.

The calendar is a sticky header component that scrolls with the user. The previous implementation of having a previous / next week component for each section caused a significant amount of visual information. Hopefully this alleviates some of that.

ChatGPT helped write some of the algorithms for comparing JS Date objects, since those aren't always super intuitive. I gave it all an additional pass to clean up / combine logic and make it more human legible.

This pull request resolves/closes/fixes WEBSITE-38.

Anything else?

As part of this work, I was exploring anchor links since the hours view is the only page we support anchor links. For example: https://deploy-preview-319--future-wwwlib-previews.netlify.app/locations-and-hours/hours-view#art-architecture-and-engineering-library

We handle anchor links in the gatsby-browser file. I extended the function to offset the anchor position by the height of the sticky header.

I also explored fixing the tab order for anchor links. In essence, when a user gets to a page via an anchor link, I want the anchored element to receive focus. Instead of trying to explore that issue within this PR, I broke it out into a separate ticket.

Testing

List instructions on how to test the pull request. Some examples:

  • Make sure the PR is consistent in these browsers:
    • Chrome
    • Firefox
    • Safari (the assignee was not able to test the pull request in this browser)
    • Edge
  • Run accessibility tests:
    • WAVE
    • ARC Toolkit
    • axe DevTools

…ate-adding-the-ability-to-navigate-via-calendar-in-Hours-View
added calendar expansion on clicking date range heading. Now I need to get the calendar view fixed under the heading, clean up the code, remove the repeated component, and see if we can sticky it on scroll.
moved the previous / next week component to its own component file and render it once at the top of the hours view versus multiple times at the top of each hours location. Also made the component sticky.
made the background color match the maize used throughout the rest of the interface and moved the sticky up 1px so there's not a gap.
User can now select week from calendar view, changed the previous and next week button to a link,  added conditional highlighting for border of week.
next / previous week now changes month if moving to a different month. Selecting a week in a different month no longer selects the week index of the current month.
Renamed the file to coincide with naming convention for panels. Added accessibility and keyboard navigation to calendar view. Updated the days positioning (sun, mon, tue, etc.) to better align with the columns.
…ia-calendar-in-Hours-View' of https://github.com/mlibrary/lib.umich.edu into WEBSITE-38-Investigate-adding-the-ability-to-navigate-via-calendar-in-Hours-View
Changed background color for calendar and working on consolidating actions.
refactored common logic into single utility function.
added esc close and instructions.
Moved the isVisible up to encapsulate entire calendar component.
accessibility updates for table view.
Copy link
Collaborator

@erinesullivan erinesullivan left a comment

Choose a reason for hiding this comment

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

The only blocker is to fix the ESLint error in line 1 in src/components/panels/hours-panel-date-viewer.js.

Everything else are just learning opportunities to reduce code, which is up to you to apply. If you choose to make the changes, I would test the code before committing.

Very nice feature, overall!

src/components/panels/hours-panel-date-viewer.js Outdated Show resolved Hide resolved
Comment on lines 381 to 390
<span css={{ background: 'white',
border: `solid 1px var(--color-neutral-200)`,
borderRadius: '4px',
boxShadow: `0 1px 1px rgba(0, 0, 0, .2), 0 2px 0 0 rgba(255, 255, 255, .7) inset;`,
display: 'inline-block',
fontFamily: 'monospace',
fontSize: '0.85rem',
marginBottom: SPACING.S,
padding: `0 ${SPACING['2XS']}` }}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that the kbd element is used in src/components/site-search.js. Might be a good opportunity to create a reusable component?

};

const handleInteraction = (event, action) => {
if (event.type === 'click' || (event.type === 'keydown' && (event.key === 'Enter' || event.key === ' '))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This same logic is also used in line 42: event.type === 'click' || (event.type === 'keydown' && (event.key === 'Enter' || event.key === ' '))

Might be a good opportunity to create a function?

src/components/panels/hours-panel-date-viewer.js Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@SalazarJosh SalazarJosh left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to review! I've made all the suggested changes. Since your last review, Emma and Heidi got some user feedback and we made two more UI edits. I'll mark those so you don't have to review all the code again. Thanks again!

>
<div
css={{
alignItems: 'center',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

center aligned all items in the HoursPanelNextPrev component

<div
key={dayIndex}
css={{
backgroundColor: getColorBasedOnDate(day),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was dynamically setting the color here if the day didn't fall in the current month. UX team found the color wasn't accessible on gold background (which is the case for last / first week of month if there are overlapping days. The suggestion was to make all the numbers the same color so that rule is now gone.

@SalazarJosh SalazarJosh marked this pull request as ready for review January 28, 2025 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants