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

Expanded lunch #168

Closed
wants to merge 14 commits into from
Closed

Conversation

Aaaaa501
Copy link
Contributor

Expanded lunch menu to cover some of jazzmans menue and wood commons

src/components/cards/LunchCard.vue Outdated Show resolved Hide resolved
font-weight: bold
font-size: .85em
text-align: center
.exLunch
Copy link
Member

@JosephShepin JosephShepin Dec 23, 2022

Choose a reason for hiding this comment

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

better class name

src/data/miscLunch.json Outdated Show resolved Hide resolved
src/data/miscLunch.json Outdated Show resolved Hide resolved
const currTime = currentDate.getHours() * 100 + currentDate.getMinutes();
if (currTime < 830) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

return currTime < 830

also currTime can be renamed to currentTime

Copy link
Member

Choose a reason for hiding this comment

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

not resolved, some logic issue here

src/components/cards/LunchCard.vue Outdated Show resolved Hide resolved
isMorning() {
const currentDate = new Date();
const currTime = currentDate.getHours() * 100 + currentDate.getMinutes();
if (currTime < 830) {
Copy link
Member

Choose a reason for hiding this comment

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

add a comment for what 830 means - what time it is

src/components/cards/LunchCard.vue Outdated Show resolved Hide resolved
src/components/cards/LunchCard.vue Outdated Show resolved Hide resolved
src/components/cards/LunchCard.vue Outdated Show resolved Hide resolved
@JosephShepin
Copy link
Member

Thanks for submitting this PR, overall it's a great idea and I like the implementation.

@JosephShepin
Copy link
Member

@Aaaaa501 when it's ready, there is a re-review button. Thanks

@JosephShepin
Copy link
Member

Screenshot 2022-12-25 at 12 55 00 PM

@Aaaaa501
Copy link
Contributor Author

Forgot to remove some testing code. Removed it now.

src/components/cards/LunchCard.vue Show resolved Hide resolved
@@ -49,19 +116,36 @@ export default {
text-align: center
margin-bottom: 4px

.breakfast
border-top: var(--color) 1.5px solid
padding: 10px 0
.lunch
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.lunch
.lunch

faChevronUp,
},
expandNums: 0,
alBreakfast: ['Bacon', 'Sausage', 'Potatoes', 'Eggs'],
Copy link
Member

Choose a reason for hiding this comment

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

better name

"Protein Box",
"Bubble Tea"
],
"Coffee and Tea": [
Copy link
Member

Choose a reason for hiding this comment

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

It seems that "sides "and "beverages" accompany cafeteria food, while PWC Jazzmans, Grill and Pizza is a separate location. what is "Coffee and Tea"? is that a location of where to get food?

Is there some way we can make it more clear where these items are? I'm thinking some location tag and icon that's like these items are here. Thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arrangement ive used is just grouped by order on the school website, but the groups correspond roughly with location. Beverages and Sides are located in the same place as the cafeteria. The grill and pizza is in the wood commons, and the 'Coffee and Tea' category is at Jazzman's. I moved the beverages and sides to the top of the list because it makes more sense in proximity to the rest of the cafeteria foods, but otherwise I think the arrangement makes sense. The locations are common knowledge in my experience, so I'm not sure how useful a location tag would be

Copy link
Member

@JosephShepin JosephShepin left a comment

Choose a reason for hiding this comment

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

Screenshot 2022-12-26 at 11 21 58 PM

I'm pretty confused by how this is organized. The title says lunch, but breakfast items are shown. Breakfast shouldn't show first because that's data that doesn't change. Then after that there are the lunch items. Opening the dropdown items reveals other items that are not lunch. Drop arrows don't seem like the best option here because the data is not less relevant as you move downward (like the upcoming events) - all the additional items are of equal relevancy more or less. I think it would make more sense to keep the lunch card the way we already have it (showing only lunch), and creating a popup modal where users can see the extended menu instead of having to click through dropdowns. Furthermore it's 11 pm at night and breakfast shouldn't be showing.

also please do not mark conversations as being resolved if they are not. Thank you.

const currTime = currentDate.getHours() * 100 + currentDate.getMinutes();
if (currTime < 830) {
return true;
}
Copy link
Member

Choose a reason for hiding this comment

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

not resolved, some logic issue here

@aw-0
Copy link
Member

aw-0 commented Dec 30, 2022

Just coming across this now - love the concept! Would there be potential to rename the entire column to something like "Food" and only display the correct meals at the right times? Like Joey said - maybe breakfast from 7-11, and lunch from 11-3?

We could also add a hyperlink to an additional page that shows an elegant version of the entire menu, and all the options like you currently have @Aaaaa501. But I think having them all there at first glance leaves everything a bit cluttered

@Aaaaa501 Aaaaa501 requested a review from JosephShepin January 4, 2023 18:43
@Aaaaa501
Copy link
Contributor Author

Aaaaa501 commented Jan 4, 2023

Not sure about the icon I'm using for the button, but otherwise does this work fine?

@aw-0
Copy link
Member

aw-0 commented Jan 4, 2023

That's really sick - great work! I'll leave it to Joey to finish up his code review - the only suggestion I have is maybe adding a title to the modal up here? Like "Full Menu"
image

@JosephShepin
Copy link
Member

Thank you for making these changes. I would like to see UI that matches our existing ui framework. I have attached some images of what i'd like to see. You can copy modal code from other places so it matches.
Screenshot 2023-01-04 at 7 37 30 PM

Screenshot 2023-01-04 at 7 38 50 PM

Additionally I think the modal would look a little more refined if the data wasn't all in one column, but spread across multiple columns for easier viewing (it can condense on mobile) - flexbox would be a good choice.

Copy link
Member

@JosephShepin JosephShepin left a comment

Choose a reason for hiding this comment

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

^^^

@aw-0 aw-0 linked an issue Jan 9, 2023 that may be closed by this pull request
@JosephShepin
Copy link
Member

closing this due to inactivity, feel free to reopen when ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants