Contributor Roles and Type #696
Conversation
jyhein
force-pushed
the
f857
branch
2 times, most recently
from
30d6645 to
0f4bad7
Compare
There was a problem hiding this comment.
This is excellent work. Great extension for the required option.
In general we are aiming to have the managers configurable via configuration functions (getColumns, getTopActions, etc). But in this case I can hardly imagine someone would need to extend this. So I think we can leave it as is and improve it if needed.
Only more significant suggestion is to move the form logic for managing the contributor roles to client side. So please check that one out. After that I would do second round, just to make sure I did not miss anything..
Thanks!
src/components/Form/Form.vue
Outdated
| let errors = {}; | ||
| this.fields.forEach((field) => { | ||
| if ( | ||
| !field.isRequired || |
There was a problem hiding this comment.
This might not be necessary, if the require logic is handled inside requireWhen?
| {{ t('manager.contributorRoles.identifier') }} | ||
| </span> | ||
| </TableColumn> | ||
| <TableColumn></TableColumn> |
There was a problem hiding this comment.
For screen reader its good to have there something.
Something like:
<span class="sr-only">{{ t('common.moreActions') }}</span>
| v-for="role in contributorRoleManagerStore.roles" | ||
| :key="role.identifier" | ||
| > | ||
| <TableCell> |
There was a problem hiding this comment.
Here lets add is-row-header. That should help with accessibility. So the screen reader knows what column to always read if you are moving across the table.
| <TableCell> | ||
| <div | ||
| class="flex justify-end" | ||
| :style="{'padding-inline-end': `${depth + 0.5}rem`}" |
There was a problem hiding this comment.
no need for depth.. that can be clean up.. that was specific to the categories which are hierarchical.
| * @param {Object|null} role - The role object. | ||
| * @return {Object} - The role form. | ||
| */ | ||
| function getContributorRoleForm(action, role) { |
There was a problem hiding this comment.
Great use of useForm here.
In many new use cases we are also using useForm to create the form, so we don't have the logic spread across php and the client side. Its also helpful in use cases when we need some specific business logic around the form which can be just conveyed via configuration. Also as you can see here it still requires to configure the form client side for specific data.
My suggestion here would be just to fetch the list of the available identifiers, since you already created API for it. And create the form client side from that data. Than you could remove the form on the server side.
Example of simple form to be created client side is probably best here - https://github.com/bozana/ui-library/pull/4/files#diff-12224feaae9a7fdc7f0b8b659bec2410e933a1ea36a2daebcb22313673ee3375
This one is not merged yet. There are some other examples on main as well if you search for initEmptyForm, but these are more complex.
|
|
||
| initEmptyForm('editContributorRole', { | ||
| action: rolesApiUrl.value, | ||
| locales: supportedLocales, |
There was a problem hiding this comment.
If you don't pass locales it should by default initialise locales based on the getSupportedFormLocales, which is better fit than the getSupportedLocales
There was a problem hiding this comment.
The form name fields should be filled in ui locales for the identifier. Is this not the way to do it?
There was a problem hiding this comment.
initEmptyForm by default configures correct locales for the form - https://github.com/pkp/ui-library/blob/main/src/composables/useForm.js#L346
There was a problem hiding this comment.
I need the translations to be in UI locales. So, which is it then: supported locales or supported form locales? What’s the difference between them?
There was a problem hiding this comment.
I was asking that @bozana the other day, also wondering which one should be used when. Rule of thumb is basically UI is for displaying data, FORM is for entering data.
These will be usually same languages. And if there are some additional languages in the UI - it would fallback to the default language if there is no translation.
But we are opened to hear thoughts from you and @ajnyga as you have been thinking about locales a lot :-).
I would be also interested to hear if someone is using different set of UI and Form languages - whats the expectation from such setup.
There was a problem hiding this comment.
The identifier is the actual data here and the names for it are just for display purposes. So, using some other list than ui locales is not good, even if form locales are usually same
There was a problem hiding this comment.
There is couple of similar use cases. Like sections or categories. All are using list of form locales.
As result if someone would configure UI in language that is not covered in forms. User would see the UI in given language and couple of these labels would fallback to primary language, which often will be english.
We are not sure about exact reasoning about the UI/Form being configured separately. But if we decide to change it - it would make sense to carefully evaluate the change across the system, so we have clear rule to apply. At this point I think its best to keep it consistent with all other similar forms, which are using that.
There was a problem hiding this comment.
Hi all, @jyhein and @jardakotesovec,
Yes, as Jarda said, for the forms i.e. data input we should use supported form locales. Those are languages the journal maintains the data/texts/settings in. They could be different than the UI locales -- Journal can choose to provide menu and actions (all OJS default/existing translations) to the users in a UI language that they are not able to provide all the translations of data/texts/settings in.
The same is with submission locales -- Journal could accept submissions or metadata in a language that they do not provide translations of data/texts/settings in. That submission language could however be selected for the UI (if it exists in OJS).
Thus, in all journal forms -- all forms that do not have anything to do with submissions -- the supported form locales should be used. (As the name says :-))
There was a problem hiding this comment.
Hi, the source of misunderstanding here is probably the reasoning why we have the UI language and Form language as separate settings. This has been in OJS for a quite long time and we did not pay much attention to it when the Submission language settings were separated.
This can maybe lead to a situation where UI is turned on for a language but no data is available for the role names because the form language was not turned on. Of course there are other possibilities for misconfiguration in OJS besides this and a journal can fix this quite easily, But it does maybe pose the question whether we need those settings separately or should the UI language just enable the forms for the language. But it is not in the scope of this issue.
So let's proceed with the form languages and maybe have this discussion somewhere else.
There was a problem hiding this comment.
Maybe just a note: if data in UI language is missing, there is always a fallback locale (for required fields) we display, for contest settings it is context primary locale, for users it is site primary locale, for submission metadata it is submission locale.
src/managers/ContributorRoleManager/useContributorRoleManagerFormAddRole.js
Outdated
Show resolved
Hide resolved
4 participants
Contributor Roles and Type
Add contributor Roles to Author
Add contributor Type to Author
Remove user group functions from Author
Remove translator from user groups
Add possibility to add more roles than author and translator
Selected contributor type changes what contributor form fields are required
More than one contributor role can be selected in the contributor form