-
-
Notifications
You must be signed in to change notification settings - Fork 794
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: Shopping list UI overhaul - add wakelock #4236
feat: Shopping list UI overhaul - add wakelock #4236
Conversation
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.
Thanks for spliting all this up. Makes it super easy to review 😊👍
Since this is all the same code as on the recipe page, could you create a component that includes the switch as well as the functionality and just throw this component on both pages?
Decreases duplicate code and makes the code of those pages a bit less convoluted.
That makes sense. It might take me a bit to figure that out since I'm new to |
No worries. If you want to tackle this for fun all good and take your time, but if this is just work for you i am happy to throw this together in 5 minutes and add this to this PR. |
new file: frontend/components/global/Wakelock.vue modified: frontend/pages/shopping-lists/_id.vue
Maybe I'm a masochist but I enjoy putting myself through learning new ways to code...haha I've pushed the changes for your review. |
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.
Check the files changed tab on GH; a few linter warnings got raised. I also added a few comments
renamed: frontend/components/global/Wakelock.vue -> frontend/components/global/WakelockSwitch.vue modified: frontend/pages/shopping-lists/_id.vue
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.
Looks good to me 🚀
Thanks for your work on this!
Ah right, i missed the code warnings.
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.
There's still a few warnings to take care of; check out the "Files changed" tab on GH and scroll down and you'll see them
Sorry for the delay. PR should be ready now. |
@Wetzel402 any reason you closed this? |
Sorry, I thought it had been merged already and was doing some pruning on my repository. |
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.
Looks all good 👍
Thanks for this @Wetzel402; it was useful to me today! :) |
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
#4077
Testing
Tested in dev container