nickslick03 / messiah-meal-planner

1 stars 0 forks source link

New day selectors #34

Closed cdrice26 closed 1 month ago

cdrice26 commented 1 month ago

I found that even with changing the day selector to have the label and checkbox split on lines, it still wasn't even close to fitting across my phone, so I pulled in my DaySelector component from another app and modified it to fit this one. I also used the same component to switch the dropdown out for a tab bar, it looks a little silly at the moment because of the sort button but once we get it moved to the search bar row we should be good.

nickslick03 commented 1 month ago

I think the new day selector component looks great in the meal queue, but I am having second thoughts about it being in the day editor. Firstly, having no title is a bit weird, and that isn't too hard to fix, but the main issue is I wanted to add a counter next to each day of the week that displays how many meals are in that day, and it would probably overflow if we implemented that in the tab bar on the day selector. I know I said I didn't like the option dropdown, but it is the best option to display the number of meals in each weekday. What are your thoughts?

nickslick03 commented 1 month ago
image
cdrice26 commented 1 month ago

I've updated the code a little which I think should help. I made it so that the height of the day selector is now a min-height, so hopefully it won't overflow vertically (let me know if it still does). I've also refactored MealContainer so that title is always a string and there is now an optional daySelector prop for passing in a react component to display under the title, and then used this in DayEditor so that there is both a title and a day selector. Does this seem like a good fix?

nickslick03 commented 1 month ago

The changes make it better, I had to get rid of "meals" when the width is within the small breakpoint, but besides that it's all good. Ready to merge if you are.

cdrice26 commented 1 month ago

Alright looks great! I also went ahead and switched the start of the week to Sunday (it turned out to be a really simple change, it was going to be a separate PR until I realized that would kind of be silly for something that small), it seems to still calculate accurately for me, if it also works for you, I am good to merge.

nickslick03 commented 1 month ago

The mealQueue was still starting on monday, I updated the addmeals function. Test it out.

cdrice26 commented 1 month ago

Alright that works, this is now merged. Good catch on the meal queue, I just saw that everything looked great in the day editor and didn't even think about the add function still using the wrong array.

nickslick03 commented 1 month ago

Yeah I was looking at the user selected meals class and saw that Monday was listed first, but in the constructor the arrays are assigned with the weekday array which starts on Sunday. My fix doesn't loop through the meals object itself but rather the weekday array, but I am wondering if it was intentional to set the values in the user selected meals object outside of the constructor and in the constructor?

cdrice26 commented 1 month ago

You're right, that's an interesting observation. I think there might have been a purpose to it back when each day was of type Meal[] but with MealReference[] I don't think it matters since they both do the same thing. Another possibility for why that occurred could be that I was trying to completely avoid typing out all the weekday names more than once in the entire app so we wouldn't have situations like the one that just occurred so went with the constructor-based assignment but then realized I can't just create an arbitrary field like that (which was not a surprise but I thought maybe since JS lets you do all sorts of wonky stuff with classes that it might let me, haha).

cdrice26 commented 1 month ago

I'm going to make a new PR that restructures this object to be based on WEEKDAYS and without redundancy. I'll also include the functions to simply the complicated useMemo call in that PR.