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

pkp/pkp-lib#7135 Multiple author affiliations #417

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

@GaziYucel GaziYucel changed the title Author Affiliations (work in progress) Multiple author affiliations Oct 9, 2024
@GaziYucel
Copy link
Contributor Author

I am waiting for a variant of the FieldBaseSuggest.vue component. After this, this PR can be wrapped up.

@GaziYucel
Copy link
Contributor Author

I have done a force push, because there was an error rebasing a couple of days ago. The PR is clean and up to date again.

@GaziYucel
Copy link
Contributor Author

GaziYucel commented Nov 20, 2024

Hi @jardakotesovec,

I have implemented everything we discussed and all seems to work.

There are some notes:

  • you mentioned that Blessie was working on a variant of FieldBaseSuggest.vue which I can use
    Add customization support for FieldBaseAutosuggest options pkp-lib#10624
  • I have used <input type="text" for input fields. It would be better to have a very simple Vue component to use. This would have the following advantages:
    • less clutter and cleaner code
    • better accessibility
  • error handling
    • this is not implemented a la OJS
    • the check I do: there should be a ROR or at least one name and the name should be the primary locale
    • if this is not true, I disable the 'Add' button
    • saving the form is still enabled
  • UI design
    • @Devika008 used a pie to show how many translations there are. I couldn't do this, because there is no component for this. I implemented "MultilingualProgress" instead
    • Devika placed the text/link "# of # translations completed" below the input fields. I moved this to above, because this makes the interface less jumpy

@GaziYucel
Copy link
Contributor Author

Hi @jardakotesovec,

I have implemented the new FieldRorAutosuggest (FieldAffiliationsRorAutoSuggest) component of @blesildaramirez.

I think the PR is ready for review now. Thanks.

@bozana: fyi

@GaziYucel
Copy link
Contributor Author

I am copying this from Mattermost to here, so it's on the list to check.

As mentioned before, I think AbortController in src/composables/useFetch.js is not working as intended. Removing or adding async / await does not help.

You can recreate this with the following two files, which are also in this PR.

  • src/components/Form/fields/FieldAffiliationsTest.stories.js
  • src/components/Form/fields/FieldAffiliationsTest.vue

@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch 2 times, most recently from c76963b to 81b0017 Compare January 20, 2025 09:49
@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch 7 times, most recently from 6465c23 to cc16dda Compare January 29, 2025 11:58
@GaziYucel GaziYucel changed the title Multiple author affiliations pkp/pkp-lib#7135 Multiple author affiliations Jan 29, 2025
@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch 6 times, most recently from e98b0ef to 4ab556d Compare February 4, 2025 10:34
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch 4 times, most recently from 3d146b2 to e6f32bf Compare February 5, 2025 11:29
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. Could you please check couple more? Should be easy updates.

if (field.name === 'affiliations') {
field.primaryLocale = activeForm.primaryLocale;
field.locales = activeForm.supportedFormLocales;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to copy this from form to the field? By looking to FieldBase I see that it should be getting these two information as props - primaryLocale and locales.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would I get these from FieldBase in my component FieldAffiliations? Can you help me with this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This means that its being passed to all fields. Including your one. So if you just add it to the FieldAffiliation props - it should become available to you.

src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
src/components/Form/fields/FieldAffiliations.vue Outdated Show resolved Hide resolved
@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch from e6f32bf to 12dea61 Compare February 5, 2025 16:12
Copy link
Collaborator

@jardakotesovec jardakotesovec left a comment

Choose a reason for hiding this comment

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

At some point we should do some screen reader accessibility testing. But don't have bandwidth for that at this point.

I think its in good enough shape to be merged. Thank you for the hard work on this!

@GaziYucel GaziYucel force-pushed the multiple-author-affiliations branch from 12dea61 to 34e219c Compare February 6, 2025 08:11
@bozana bozana merged commit b5532e7 into pkp:main Feb 6, 2025
3 of 5 checks passed
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.

3 participants