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.

Sign up for GitHub

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

Add overridable tags #287

Merged
merged 9 commits into from
Apr 8, 2024
Merged

Add overridable tags #287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8db39eb
Add overridable tags
SarahW91 Dec 7, 2023
dd898a2
Adjust overridable ids
SarahW91 Dec 7, 2023
3dd61ea
Correct overridable tag containing multiple children
SarahW91 Dec 11, 2023
80a1591
Rename Overridable IDs.
SarahW91 Apr 3, 2024
908e329
Rename Overridable IDs again.
SarahW91 Apr 3, 2024
55f16c5
Merge branch 'inveniosoftware:master' into add-overridable-tags
SarahW91 Apr 3, 2024
37e7846
Update invenio_vocabularies/assets/semantic-ui/js/invenio_vocabularie…
SarahW91 Apr 4, 2024
9c3dba9
Formatting with prettier.
SarahW91 Apr 4, 2024
8093de8
Add fieldPath to Overridable tags.
SarahW91 Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 67 additions & 56 deletions ...s/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { TextField, RemoteSelectField } from "react-invenio-forms";
import { i18next } from "@translations/invenio_rdm_records/i18next";
import _isEmpty from "lodash/isEmpty";

import Overridable from "react-overridable";

function CustomAwardForm({ deserializeFunder, selectedFunding }) {
function deserializeFunderToDropdown(funderItem) {
let funderName = null;
Expand Down Expand Up @@ -48,64 +50,73 @@ function CustomAwardForm({ deserializeFunder, selectedFunding }) {

return (
<Form>
<RemoteSelectField
fieldPath="selectedFunding.funder.id"
suggestionAPIUrl="/api/funders"
suggestionAPIHeaders={{
Accept: "application/vnd.inveniordm.v1+json",
}}
placeholder={i18next.t("Search for a funder by name")}
serializeSuggestions={(funders) => {
return funders.map((funder) =>
deserializeFunderToDropdown(deserializeFunder(funder))
);
}}
searchInput={{
autoFocus: _isEmpty(selectedFunding),
}}
label={i18next.t("Funder")}
noQueryMessage={i18next.t("Search for funder...")}
clearable
allowAdditions={false}
multiple={false}
selectOnBlur={false}
selectOnNavigation={false}
required
search={(options) => options}
isFocused
onValueChange={({ formikProps }, selectedFundersArray) => {
if (selectedFundersArray.length === 1) {
const selectedFunder = selectedFundersArray[0];
if (selectedFunder) {
const deserializedFunder = serializeFunderFromDropdown(selectedFunder);
formikProps.form.setFieldValue(
"selectedFunding.funder",
deserializedFunder
);
<Overridable id="InvenioVocabularies.CustomAwardForm.RemoteSelectField.SelectedFundingFunderId">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.CustomAwardForm.RemoteSelectField.SelectedFundingFunderId">
<Overridable id="InvenioVocabularies.CustomAwardForm.container">

<RemoteSelectField
fieldPath="selectedFunding.funder.id"
suggestionAPIUrl="/api/funders"
suggestionAPIHeaders={{
Accept: "application/vnd.inveniordm.v1+json",
}}
placeholder={i18next.t("Search for a funder by name")}
serializeSuggestions={(funders) => {
return funders.map((funder) =>
deserializeFunderToDropdown(deserializeFunder(funder))
);
}}
searchInput={{
autoFocus: _isEmpty(selectedFunding),
}}
label={i18next.t("Funder")}
noQueryMessage={i18next.t("Search for funder...")}
clearable
allowAdditions={false}
multiple={false}
selectOnBlur={false}
selectOnNavigation={false}
required
search={(options) => options}
isFocused
onValueChange={({ formikProps }, selectedFundersArray) => {
if (selectedFundersArray.length === 1) {
const selectedFunder = selectedFundersArray[0];
if (selectedFunder) {
const deserializedFunder = serializeFunderFromDropdown(selectedFunder);
formikProps.form.setFieldValue(
"selectedFunding.funder",
deserializedFunder
);
}
}
}
}}
/>

<Header as="h3" size="small">
{i18next.t("Award information")} ({i18next.t("optional")})
</Header>
<Form.Group widths="equal">
<TextField
label={i18next.t("Number")}
placeholder={i18next.t("Award number")}
fieldPath="selectedFunding.award.number"
/>
<TextField
label={i18next.t("Title")}
placeholder={i18next.t("Award Title")}
fieldPath="selectedFunding.award.title"
/>
<TextField
label={i18next.t("URL")}
placeholder={i18next.t("Award URL")}
fieldPath="selectedFunding.award.url"
}}
/>
</Overridable>
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardInformation.Header">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardInformation.Header">
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardInformation.Header.container">

<Header as="h3" size="small">
{i18next.t("Award information")} ({i18next.t("optional")})
</Header>
</Overridable>
<Form.Group widths="equal">
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.TextField">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.TextField">
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.container">

<TextField
label={i18next.t("Number")}
placeholder={i18next.t("Award number")}
fieldPath="selectedFunding.award.number"
/>
</Overridable>
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.TextField">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.TextField">
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.container">

<TextField
label={i18next.t("Title")}
placeholder={i18next.t("Award Title")}
fieldPath="selectedFunding.award.title"
/>
</Overridable>
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.TextField">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.TextField">
<Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.container">

<TextField
label={i18next.t("URL")}
placeholder={i18next.t("Award URL")}
fieldPath="selectedFunding.award.url"
/>
</Overridable>
</Form.Group>
</Form>
);
Expand Down
88 changes: 48 additions & 40 deletions ...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/FundingField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import FundingModal from "./FundingModal";

import { i18next } from "@translations/invenio_rdm_records/i18next";

import Overridable from "react-overridable";

function FundingFieldForm(props) {
const {
label,
Expand Down Expand Up @@ -108,46 +110,52 @@ function FundingFieldForm(props) {
);
})}
</List>
<FundingModal
searchConfig={searchConfig}
trigger={
<Button
type="button"
key="custom"
icon
labelPosition="left"
className="mb-5"
>
<Icon name="add" />
{i18next.t("Add award")}
</Button>
}
onAwardChange={(selectedFunding) => {
formikArrayPush(selectedFunding);
}}
mode="standard"
action="add"
deserializeAward={deserializeAward}
deserializeFunder={deserializeFunder}
computeFundingContents={computeFundingContents}
/>
<FundingModal
searchConfig={searchConfig}
trigger={
<Button type="button" key="custom" icon labelPosition="left">
<Icon name="add" />
{i18next.t("Add custom")}
</Button>
}
onAwardChange={(selectedFunding) => {
formikArrayPush(selectedFunding);
}}
mode="custom"
action="add"
deserializeAward={deserializeAward}
deserializeFunder={deserializeFunder}
computeFundingContents={computeFundingContents}
/>

<Overridable id="InvenioVocabularies.FundingField.AddAward.Button">
Copy link
Contributor

@kpsherva kpsherva Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.FundingField.AddAward.Button">
<Overridable id="InvenioVocabularies.FundingField.actions.container">

I would go with a common container for the actions of this field, rather than wrapping each element separately. There might be more action buttons that can be added to this field, and common container allows for more flexibility as well as it reduces the amount of unique overridable IDs that we need to maintain in the code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually need these separate ids on individual elements since we just want to hide one of these buttons (we only want to allow adding custom awards in our project)

Copy link

Choose a reason for hiding this comment

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

I think there would be less flexibility in wrapping the common container. You would have to override each element (including those you want to keep) and even the possible future elements, wouldn't you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your point, and you are right on the fact we should allow overriding each of the elements as well. In this case, however, the overridable ID should be added in the component itself - either on the <Button> in line 118 or inFundingModal. While we are adding overridable IDs as wrappers (parent of several "subcomponents") then we identify it as a "container" - otherwise we have

InvenioVocabularies.FundingField.AddAward.Button
InvenioVocabularies.FundingField.AddCustom.Button

both wrapping two instantiations of FundingModal component, which is semantically incorrect - the naming does not mention the modal itself whatsoever. This naming convention we reserved to be associated with a component definition, not instantiation, to avoid misunderstandings and inconsistencies in the naming.

I am happy to explain it more in details on how this naming convention was planned to be used over a call.

<FundingModal
searchConfig={searchConfig}
trigger={
<Button
type="button"
key="custom"
icon
labelPosition="left"
className="mb-5"
>
<Icon name="add" />
{i18next.t("Add award")}
</Button>
}
onAwardChange={(selectedFunding) => {
formikArrayPush(selectedFunding);
}}
mode="standard"
action="add"
deserializeAward={deserializeAward}
deserializeFunder={deserializeFunder}
computeFundingContents={computeFundingContents}
/>
</Overridable>

<Overridable id="InvenioVocabularies.FundingField.AddCustom.Button">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Overridable id="InvenioVocabularies.FundingField.AddCustom.Button">
<Overridable id="InvenioVocabularies.FundingField.AddCustom.container">

<FundingModal
searchConfig={searchConfig}
trigger={
<Button type="button" key="custom" icon labelPosition="left">
<Icon name="add" />
{i18next.t("Add custom")}
</Button>
}
onAwardChange={(selectedFunding) => {
formikArrayPush(selectedFunding);
}}
mode="custom"
action="add"
deserializeAward={deserializeAward}
deserializeFunder={deserializeFunder}
computeFundingContents={computeFundingContents}
/>
</Overridable>
</Form.Field>
</DndProvider>
);
Expand Down
Loading