search

nickslick03 / messiah-meal-planner

1 stars 0 forks source link

Bottom half of frontend (MealQueue and DayEditor) #16

Closed nickslick03 closed 1 month ago

nickslick03 commented 1 month ago

I created MealQueue and DayEditor. Let me know what you think.

nickslick03 commented 1 month ago

I'm gonna make a big refactoring change so don't touch the branch for a bit

nickslick03 commented 1 month ago

I created a component called MealContainer, which is Section and MealTable component combined, so the components that need a meal table DRY. I know it's a lot in one PR but just try the UI first, the add/remove buttons produces console log statements to certify it's working

cdrice26 commented 1 month ago

Looks nice overall. Before we merge it would be good to have documentation on all these new components though, it took some time to go through it all to figure out how it works. I updated some docs but didn't add new ones. I definitely think the grand total and under/over need to feature more prominently as opposed to just being in the day editor but the bottom bar you proposed should fix this (might be good to have that be a separate PR?). Other than that I think we're good.

nickslick03 commented 1 month ago

I forgot documentation, my bad. I'll get on those. The results container and mini results bar are seperate from the day editor, so that'll be another PR.

nickslick03 commented 1 month ago

Added documentation

cdrice26 commented 1 month ago

Okay, I've reviewed the documentation. I added some in DayEditor since it was still missing, and found the JSDoc on DotLeader to not represent the component overly well, so I edited that one and added comments to state variables and functions. Other than that it all looks great. I did make a few minor code changes. I changed the dot leader to use a dynamic number of dots based on screen width, and I discovered that the tables with a "remove" column were too wide on mobile to fit well, so I changed the column header from 'Remove' to 'Del' for now. It's fairly clear what that means though it is also the only abbreviated header which I'm not sold on, if you have a better idea feel free to implement it. Otherwise, I think it's good to merge.

nickslick03 commented 1 month ago

I like where you're going by making the dots relative to the screen width, but what if the user flips the phone sideways or loads the website in a half-width window and makes the window take up full width? tracking the width and updating the dot variable feels like more work than necessary, so I think keeping the dot variable as a long string of dots is the best option even if it feels like a hack. I'm good with all the other changes, but after this sprint, I have a list of small UI blemishes I want to look at, one of them being the overflow on the meal tables with small widths, but making delete into del is a good start.

cdrice26 commented 1 month ago

Oh that makes sense. Switched it back to a static 200 dots, but still using the repeat function to clean it up a bit.