-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Feat/recipe timeline event UI #1831
Feat/recipe timeline event UI #1831
Conversation
set time to 1 minute before midnight adjusted display to properly interpret UTC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things in the frontend but looks good overall. Given the previous merged, does anything here need to change?
Thanks! I'll take a look at your comments - looking briefly they make sense. |
@hay-kot this should be ready :) |
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This implements a new UI for recipe timeline events, and fixes a few bugs and visual glitches with the new "I Made This" functionality (#1801). With these changes we more-or-less complete #1451, save for implementing image handling (this PR is big enough already, I'll add that later).
To open the new UI, there's a new badge:
Here's what it looks like on desktop:
And on mobile we use the dense view:
If you are the "owner" of the event (your
userId
matches the event'suserId
") and theeventType
is notsystem
, then there's a context menu that lets you edit or delete the event:If no events are on the recipe, you get a generic message instead. Note that this shouldn't happen with new recipes, as they always create an event since that API was implemented. To trigger this I had to delete the default event through the API.
The already-implemented "I Made This" UI rendered horribly on mobile, so that has been updated.
Desktop:
Mobile:
Lastly, I made all of this text localizable.
Which issue(s) this PR fixes:
(REQUIRED)
Completes FR #1451
Special notes for your reviewer:
Ooh boy did I learn a lot about Vue and JS working on this. Happy to fix/refactor anything that doesn't make sense or can look better. I tried to mirror the existing components as best as possible.
Did I implement the localizations correctly? Will the JSON be automatically updated next time the Crowdin code runs? I mirrored a different PR I found that added new localizations, and I think I did it right.
In a few places I had to do some cheeky stuff to convert between local datetimes and UTC datetimes. What I did definitely works, but my Python brain had trouble wrapping my head around how JS handles datetimes. It's worth reviewing and seeing if I cooked too much pasta with the datetime stuff, specifically in
RecipeLastMade.vue
andRecipeDialogTimeline.vue
.Testing
(fill-in or delete this section)
I tested the following functionalities:
I tested the following elements:
Release Notes
(REQUIRED)