-
Notifications
You must be signed in to change notification settings - Fork 213
Feedback utility functions #5088
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
base: unstable
Are you sure you want to change the base?
Feedback utility functions #5088
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.
Logically, everything in the code makes sense! Just a few small tweaks and some debugging is needed to resolve the UI issues encountered during testing. Additionally, I have a few thoughts on potential optimizations we could explore further.
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...tion/contentcuration/frontend/RecommendedResourceCard/components/RecommendedResourceCard.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
a8759bb
to
02b5952
Compare
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.
Unless I'm missing something, the event ordering seems misaligned.
Additionally, there's a confusing mix of async/await, old-style Promise chaining, and some ignored promises. I suggest some consistency, with code comments for explaining the purposes of any inconsistencies-- if a promise is ignored, give a reason. For new code, I see no reason not to use async/await
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
...ation/contentcuration/frontend/channelEdit/views/ImportFromChannels/SearchOrBrowseWindow.vue
Outdated
Show resolved
Hide resolved
5e48e3d
to
c793fc9
Compare
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.
I really like the updates to the code, @akolson! Nice work. Feels much cleaner and localized. Let's test it on the unstable server
Summary
This pr does the following;
References
Fixes #5057
Fixes #5058
Fixes #5088
Reviewer guidance
RecommendationsEvent
andRecommendationsInteractionEvent
models