Skip to content

Add popover to indicate the current meal block period#712

Draft
laasyaaki wants to merge 15 commits intostagingfrom
block_periods
Draft

Add popover to indicate the current meal block period#712
laasyaaki wants to merge 15 commits intostagingfrom
block_periods

Conversation

@laasyaaki
Copy link
Copy Markdown
Collaborator

#706

based on feedback form request

@vercel
Copy link
Copy Markdown

vercel bot commented Oct 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
cmueats Ready Ready Preview Dec 19, 2025 5:47pm

@laasyaaki laasyaaki marked this pull request as ready for review October 21, 2025 19:08
@laasyaaki laasyaaki requested a review from cirex-web October 21, 2025 19:09
@cirex-web
Copy link
Copy Markdown
Collaborator

cirex-web commented Oct 30, 2025

oh yea also I have like three midterms next week 😭 so feel free to approve and merge other ppl's PRs without my approval. I think main thing is just make sure the class names are named well and that css modules are used whenever possible.

@GhostOf0days
Copy link
Copy Markdown
Member

試験がんばってね、エリック!
image

@cirex-web
Copy link
Copy Markdown
Collaborator

cirex-web commented Nov 10, 2025

I like how you bold the active row here!
image

thoughts on this revision of your design? (filter has been moved to the right, closer to the search bar, which makes more sense I think) I also added a button so ppl on mobile can click on it (actually we can probably make the whole row clickable and hoverable)
Figma link: https://www.figma.com/design/dp9RzhJtoMFWiUGjVCd6K7/CMUEats?node-id=1626-13703&t=ziZxOqbwwu2MDqOy-11

image image image image

Comment on lines +59 to +61
const now = new Date();
const pittsburghTime = new Date(now.toLocaleString('en-US', { timeZone: 'America/New_York' }));
const seconds = pittsburghTime.getHours() * 3600 + pittsburghTime.getMinutes() * 60 + pittsburghTime.getSeconds();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm 90% sure this doesn't do what you think it does - this is probably equivalent to const pittsbughTime = new Date() (and new Date().getHours() defaults to whatever timezone your computer is set to)

You can use the luxon package's DateTime.now().setZone('America/New_York') instead to force the timezone to be in pittsburgh time, no matter what the computer's local timezone is

Copy link
Copy Markdown
Collaborator Author

@laasyaaki laasyaaki Nov 15, 2025

Choose a reason for hiding this comment

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

I made this in MN (cst) and it worked out fine then! but i'll test a bit more

const shouldAnimateCards = useRef(true);

const blockPeriods = getBlockPeriodsWithTimes();
const currentPeriod = getBlockPeriod();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

might be good to make getBlockPeriod a pure function that takes in the current time instead. (helps with testing and re-rendering if we decide to periodically update what "now" represents)

{ period: 'Dinner', timeRange: '04:30 PM - 08:59 PM' },
{ period: 'Late Night', timeRange: '09:00 PM - 03:29 AM' },
];
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

any reason why this is decoupled from

    const breakfastStart = 3 * 3600 + 30 * 60;
    const lunchStart = 10 * 3600 + 30 * 60;
    const dinnerStart = 16 * 3600 + 30 * 60;
    const lateNightStart = 21 * 3600;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

math is hard :/

Comment on lines +198 to +199
<div onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave} className="block-period">
<p className={`block-period__current ${isPopupVisible ? 'block-period__current--hidden' : ''}`}>
Copy link
Copy Markdown
Collaborator

@cirex-web cirex-web Nov 10, 2025

Choose a reason for hiding this comment

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

might be good to extract all of this into a separate component/file (its functionality is completely separate from everything else)

@laasyaaki
Copy link
Copy Markdown
Collaborator Author

image

I like the buttons you added i think it could be helpful for mobile!
i'm not really sure how I like this design because it looks a bit out of place from the cmueats theme, and on mobile it kind of seperates the screen, but we can get some more opinions on what's more user friendly!

@cirex-web
Copy link
Copy Markdown
Collaborator

cirex-web commented Nov 16, 2025

What specifically about this feels off?

image

and yea maybe we can pick a better font but I think having a monospaced one helps with readability
image

or ig this (Inter) is fine
image

How about de-emphasizing the text a bit for mobile?
image

or maybe two lines?
image

@cirex-web
Copy link
Copy Markdown
Collaborator

cirex-web commented Nov 17, 2025

or this?
image
image

@laasyaaki
Copy link
Copy Markdown
Collaborator Author

What specifically about this feels off?

image

I prefer the popup under the searchbar, and I don't really like how it disrupts the margin and comes in from the right. But I like your idea to have a different font, I like the monospaced font.

@laasyaaki
Copy link
Copy Markdown
Collaborator Author

or this? image image

I like this! I just think the header should be the first thing people see and the block periods popup should be with the rest of the filters and I would make it the dark gray color of the rest of the filtering and search stuff

@cirex-web
Copy link
Copy Markdown
Collaborator

I just think the header should be the first thing people see and the block periods popup should be with the rest of the filters and I would make it the dark gray color of the rest of the filtering and search stuff

you don't think that making it the same color as the filtering and search might cause confusion? cuz it's not really part of the search functionality lol

@laasyaaki
Copy link
Copy Markdown
Collaborator Author

I just think the header should be the first thing people see and the block periods popup should be with the rest of the filters and I would make it the dark gray color of the rest of the filtering and search stuff

you don't think that making it the same color as the filtering and search might cause confusion? cuz it's not really part of the search functionality lol

no lol I think it's all part of the side functionality and not the main eatery card functionality. Also, If its on the other side there's some space between the block period popover and the other filters. Maybe we can switch the sides of the filters to group all of the filters together, but I still think the popover should follow the same color scheme and styling as the filtering stuff. we can poll slack?

@cirex-web
Copy link
Copy Markdown
Collaborator

but I still think the popover should follow the same color scheme and styling as the filtering stuff. we can poll slack?

sure

@cirex-web
Copy link
Copy Markdown
Collaborator

cirex-web commented Dec 13, 2025

Very cool, thanks!

  • Why is the font size so small on mobile?
image
  • Maybe we can have the entire popup expand on desktop on hover, rather than on click (at least that was what I was thinking lol)
  • Can we close the box on tap anywhere outside the box as well? I used composedPath for detecting clicks outside the dropdown in a chrome extension I built last month, if you want an example
  // close dropdown when an outside click occurs
  useEffect(() => {
    if (!isOpen) return;
    const onClick = (ev: PointerEvent) => {
      if (dropdownRef.current === null || plusButtonRef.current === null)
        return;
      if (
        !ev.composedPath().includes(dropdownRef.current) &&
        !ev.composedPath().includes(plusButtonRef.current)
      ) {
        onClose();
      }
    };
    document.body.addEventListener("click", onClick);
    return () => document.body.removeEventListener("click", onClick);
  }, [isOpen]);

@cirex-web
Copy link
Copy Markdown
Collaborator

hi Laasya! I can wrap this up if you're busy over the next few days

@laasyaaki
Copy link
Copy Markdown
Collaborator Author

hi Laasya! I can wrap this up if you're busy over the next few days

Hey I can wrap this up, I'm sorry I didn't see your comment on my latest changes

@Arom1a
Copy link
Copy Markdown
Contributor

Arom1a commented Dec 24, 2025

The div is too wide so that hovering on any area will trigger the expansion

Snipaste_2025-12-24_10-54-59 Snipaste_2025-12-24_10-51-17

@cirex-web cirex-web marked this pull request as draft December 25, 2025 19:06
Base automatically changed from staging to main January 2, 2026 02:56
@cirex-web cirex-web changed the base branch from main to staging January 2, 2026 03:03
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.

4 participants