Skip to content
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

Improved contributor modal hash and support for new authors #153

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TheRolfFR
Copy link
Member

Fixes #122 with better hash and added support for new authors in contributor modal

@TheRolfFR TheRolfFR requested a review from 3vorp January 12, 2025 23:07
@TheRolfFR TheRolfFR force-pushed the fix/contributor_modal_names_duplicated branch from 6286dc0 to c7a281b Compare January 12, 2025 23:14
Copy link
Contributor

@3vorp 3vorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a huge fan of using requests in watchers and computed since it can get a bit api-spammy and cause some render delay—can't you just fetch all the contributors once (iirc contributors already does this) and get by author key instead for O(1) lookup?

pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
pages/contribution/contribution-modal.vue Outdated Show resolved Hide resolved
const notAlreadyAuthorsIds = authorIds.filter((id) => alreadyAuthorsIds.indexOf(id) === -1);
const searchIDsparam = notAlreadyAuthorsIds.join(',')
const notAlreadyAuthorsFound = (!searchIDsparam) ? [] :
await axios.get(`${this.$root.apiURL}/users/${searchIDsparam}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't contributors already cached? why do we need to re-fetch everything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future new authors aren't, user select fetches them online but forgets to emit it to form and modal

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would be smarter to fetch every user when the component is created then filter relevant results using a computed property

pages/contribution/contribution-modal.vue Show resolved Hide resolved
@TheRolfFR TheRolfFR requested a review from 3vorp January 14, 2025 18:27
Copy link
Contributor

@3vorp 3vorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make more sense to fetch every user when the component is created (using https://api.faithfulpack.net/v2/users/names), then filter relevant results using a computed property—that way you avoid the request overhead/api spam and you still have fast contribution adding using the user-select component

oh also please run prettier on your code before merging (nicer diff)

@@ -65,6 +65,7 @@
:error-messages="
content.length === 0 ? [$root.lang('database.subtitles.no_contributor_yet')] : []
"
@newUsers="(l) => this.$emit('newUser', l) "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be @newUser according to the user-select code I think

@@ -50,7 +50,7 @@
<v-list-item-title>{{ panelLabels[form.formId] }}</v-list-item-title>
<v-list-item-subtitle class="text-truncate">
<span v-if="form.authors.length">
{{ contributorsFromIds(form.authors) }}
{{ formAuthorNames[form.formId] || '...' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use "…" instead of "...", bit nicer

const notAlreadyAuthorsIds = authorIds.filter((id) => alreadyAuthorsIds.indexOf(id) === -1);
const searchIDsparam = notAlreadyAuthorsIds.join(',')
const notAlreadyAuthorsFound = (!searchIDsparam) ? [] :
await axios.get(`${this.$root.apiURL}/users/${searchIDsparam}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would be smarter to fetch every user when the component is created then filter relevant results using a computed property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Display issue: Authors in the Create Contribution modal sometimes get duplicated
2 participants