Add overridable tags #287

SarahW91 · 2023-12-11T13:38:01Z

Description

Added additional Overridable tags for a more finegrained control over React components.

Checklist

I'm aware of the code of conduct.
I've created logical separate commits and followed the commit message format.
I've followed the CSS/JS and React guidelines (for relevant code).
I've NOT included third-party code (copy/pasted source code or new dependencies).

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

You license your contribution under the same terms as the current repository’s license.
You agree that you have the right to license your contribution under the current repository’s license.

kpsherva · 2023-12-14T14:01:03Z

...ularies/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/FundingField.js

-          computeFundingContents={computeFundingContents}
-        />
+
+        <Overridable id="InvenioVocabularies.FundingField.AddAward.Button">


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

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)

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?

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.

kpsherva

Hello @SarahW91 I was reviewing both PRs, please check my comments in
inveniosoftware/invenio-rdm-records#1631 (review)
I believe we could better align over a zoom call. We could try to schedule it on discord. Please let me know what you think :)

kpsherva · 2024-03-21T15:19:31Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

-                "selectedFunding.funder",
-                deserializedFunder
-              );
+      <Overridable id="InvenioVocabularies.CustomAwardForm.RemoteSelectField.SelectedFundingFunderId">


Suggested change

<Overridable id="InvenioVocabularies.CustomAwardForm.RemoteSelectField.SelectedFundingFunderId">

<Overridable id="InvenioVocabularies.CustomAwardForm.container">

kpsherva · 2024-03-21T15:20:07Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

        />
+      </Overridable>
+      <Overridable id="InvenioVocabularies.CustomAwardForm.AwardInformation.Header">


Suggested change

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardInformation.Header">

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

kpsherva · 2024-03-21T15:20:17Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

+        </Header>
+      </Overridable>
+      <Form.Group widths="equal">
+        <Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.TextField">


Suggested change

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.TextField">

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardNumber.container">

kpsherva · 2024-03-21T15:20:28Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

+            fieldPath="selectedFunding.award.number"
+          />
+        </Overridable>
+        <Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.TextField">


Suggested change

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.TextField">

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitle.container">

kpsherva · 2024-03-21T15:20:39Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

+            fieldPath="selectedFunding.award.title"
+          />
+        </Overridable>
+        <Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.TextField">


Suggested change

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.TextField">

<Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrl.container">

kpsherva · 2024-03-21T15:22:18Z

...ularies/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/FundingField.js

+          />
+        </Overridable>
+
+        <Overridable id="InvenioVocabularies.FundingField.AddCustom.Button">


Suggested change

<Overridable id="InvenioVocabularies.FundingField.AddCustom.Button">

<Overridable id="InvenioVocabularies.FundingField.AddCustom.container">

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

…s/src/contrib/forms/Funding/CustomAwardForm.js Add fieldPath attribute to Overridable tag Co-authored-by: Karolina <[email protected]>

kpsherva · 2024-04-05T11:58:11Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

+            fieldPath="selectedFunding.award.number"
+          />
+        </Overridable>
+        <Overridable id="InvenioVocabularies.CustomAwardForm.AwardTitleTextField.Container">


suggestion: please apply the same as in the component above

kpsherva · 2024-04-05T11:58:14Z

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js

+            fieldPath="selectedFunding.award.title"
+          />
+        </Overridable>
+        <Overridable id="InvenioVocabularies.CustomAwardForm.AwardUrlTextField.Container">


suggestion: please apply the same as in the component above

SarahW91 added 2 commits December 7, 2023 15:24

Add overridable tags

8db39eb

Adjust overridable ids

dd898a2

SarahW91 requested a review from kpsherva December 11, 2023 13:38

SarahW91 closed this Dec 11, 2023

Correct overridable tag containing multiple children

3dd61ea

SarahW91 reopened this Dec 11, 2023

kpsherva reviewed Dec 14, 2023

View reviewed changes

kpsherva reviewed Mar 6, 2024

View reviewed changes

kpsherva reviewed Mar 21, 2024

View reviewed changes

SarahW91 and others added 3 commits April 3, 2024 10:18

Rename Overridable IDs.

80a1591

Rename Overridable IDs again.

908e329

Merge branch 'inveniosoftware:master' into add-overridable-tags

55f16c5

kpsherva reviewed Apr 4, 2024

View reviewed changes

...ries/assets/semantic-ui/js/invenio_vocabularies/src/contrib/forms/Funding/CustomAwardForm.js Outdated Show resolved Hide resolved

SarahW91 and others added 2 commits April 4, 2024 13:52

Update invenio_vocabularies/assets/semantic-ui/js/invenio_vocabularie…

37e7846

…s/src/contrib/forms/Funding/CustomAwardForm.js Add fieldPath attribute to Overridable tag Co-authored-by: Karolina <[email protected]>

Formatting with prettier.

9c3dba9

kpsherva reviewed Apr 5, 2024

View reviewed changes

Add fieldPath to Overridable tags.

8093de8

kpsherva approved these changes Apr 8, 2024

View reviewed changes

kpsherva merged commit f6e6ffd into inveniosoftware:master Apr 8, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overridable tags #287

Add overridable tags #287

SarahW91 commented Dec 11, 2023

kpsherva Dec 14, 2023 •

edited

Loading

SarahW91 Dec 14, 2023

mkloeppe Jan 15, 2024

kpsherva Mar 6, 2024

kpsherva left a comment

kpsherva Mar 21, 2024

kpsherva Mar 21, 2024

kpsherva Mar 21, 2024

kpsherva Mar 21, 2024

kpsherva Mar 21, 2024

kpsherva Mar 21, 2024

kpsherva Apr 5, 2024

kpsherva Apr 5, 2024

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

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

Add overridable tags #287

Add overridable tags #287

Conversation

SarahW91 commented Dec 11, 2023

Description

Checklist

kpsherva Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsherva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpsherva Dec 14, 2023 •

edited

Loading