-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feature #741: Use opaque cursor on submission list and entity list #851
Feature #741: Use opaque cursor on submission list and entity list #851
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.
This is a great simplification! I love that we no longer need to track instance IDs or have challenges related to scrolling in chunks while there's concurrent creation or deletion. The code here definitely looks like it's on the right track. I've left some suggestions, but I think @ktuite could do the final review without me if that ends up working best. Excited for $skiptoken
and the changes here!
if (props.totalCount <= props.top) | ||
return tcn(`${props.type}.all`, props.totalCount); | ||
|
||
// console.log( t('submission.first', { count: 'asdf', top: 10 }, 11 ) ); |
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.
delete this
src/util/i18n.js
Outdated
@@ -43,3 +43,8 @@ export const loadLocale = ({ i18n, logger }, locale) => { | |||
export function $tcn(path, count, values = undefined) { | |||
return this.$tc(path, count, { count: this.$n(count, 'default'), ...values }); | |||
} | |||
|
|||
// same as above but for composition API | |||
export function tcn(path, count, values) { |
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.
double check if I really need this function
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.
Looked at this after a synchronous review with Sadiq earlier and I think it's good!
Closes #741
What has been done to verify that this works as intended?
Integration tests and manual verification
Why is this the best possible solution? Were any other approaches considered?
Used the same approach as Submissions List
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
NA
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
No
Before submitting this PR, please make sure you have:
npm run test
andnpm run lint
and confirmed all checks still pass OR confirm CircleCI build passes