NEW Add MultiLinkField #120

GuySartorelli · 2023-11-28T03:53:40Z

Notes

I've refactored some of the original components to avoid code duplication - most of this can be seen in the first commit.
Looking at the existing unit test coverage, there's nothing particular about this PR that needs to have new unit tests. I suspect a majority of it will be covered by behat tests later on instead, which is a separate card.

Issue

Multiple link support #107

GuySartorelli · 2023-11-28T03:59:21Z

client/src/entwine/LinkField.js

-      Root.unmount();
+      if (Root) {
+        Root.unmount();
+      }


I was sometimes getting console errors about unmount not being a function on undefined.
See https://docs.silverstripe.org/en/5/changelogs/5.0.0/#reactdom-render-replaced-with-reactdom-createroot-render where this pattern of checking for the root before unmounting is documented as best practice anyway.

GuySartorelli · 2023-11-28T22:13:57Z

babel.config.json

These presets are already defined in the global webpack config.
See https://github.com/silverstripe/webpack-config/blob/5832637ef465be20afd50407cdae0185880166b5/js/modules.js#L15-L25

GuySartorelli · 2023-11-28T22:15:18Z

.eslintrc.js

+  // Allows null coalescing and optional chaining operators.
+  parserOptions: {
+    ecmaVersion: 2020
+  },


Null coallescing (x ?? y) and optional chaining (x?.y) are well supported now - the build would have failed if they weren't supported by the browsers we target, due to the browser config in package.json

GuySartorelli · 2023-11-28T22:16:16Z

client/src/components/LinkField/LinkField.js

This component now handles single and multi link fields.
I originally had them as two separate components, but the differences were so minimal it just made more sense to have a isMulti boolean prop and let the one component handle both scenarios.

Note that I've moved the LinkPickerTitle out of the picker, so that it can be used to render the links when linkfield is multi.

The picker is now responsible for knowing what type was picked, and passing it through to the modal. The linkfield shouldn't care about that, it only cares about the link once the link has been created.

I've also moved the responsibility of rendering the modal. If it's a new link, the modal is rendered by the picker (indirectly, through the new LinkModalContainer). The linkfield itself only renders modals for links that are being edited.

Finally, the logic for figuring out which link modal component to render was moved into LinkModalContainer so it didn't get duplicated between the linkfield and the picker components.

GuySartorelli · 2023-11-28T22:21:25Z

client/src/components/LinkModal/LinkModal.js

+  /**
+   * Call back used by LinkModal after the form has been submitted and the response has been received
+   */
+  const onSubmit = async (modalData, action, submitFn) => {


Moved a bunch of this logic from linkfield into here - linkfield doesn't care that the modal was submitted, it only cares about success. Similarly, the modal should be responsible for knowing about validation errors - linkfield itself doesn't care about them.

GuySartorelli · 2023-11-28T22:22:29Z

client/src/components/LinkPicker/LinkPickerTitle.js

The only change to LinkPickerTitle was to give it the classnames necessary for it to look correct when rendered outside of the picker component.

GuySartorelli · 2023-11-28T22:23:46Z

package.json

-    "@silverstripe/eslint-config": "^1.0.0-alpha6",
-    "@silverstripe/webpack-config": "^2.0.0-alpha9",
+    "@silverstripe/eslint-config": "^1.0.0",
+    "@silverstripe/webpack-config": "^2.0.0",


Shouldn't be using alpha anymore.

GuySartorelli · 2023-11-28T23:35:11Z

client/src/components/LinkField/LinkField.js

-  value: PropTypes.number.isRequired,
+  value: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.number), PropTypes.number]),


Can't be required, because the value is undefined when no links are in the field and the field is rendered in an elemental block.
The component handles undefined perfectly so that's not a problem.

GuySartorelli · 2023-11-29T00:10:34Z

client/src/components/LinkPicker/LinkPicker.scss

I've reduced all of the &__ down to their full .link-picker__ forms so that we can easily do a text search for any given CSS class and find the style here.

GuySartorelli · 2023-11-29T00:11:35Z

client/src/components/LinkPicker/LinkPickerMenu.js

-      <DropdownToggle className="link-picker__menu-toggle font-icon-link" caret>{i18n._t('LinkField.ADD_LINK', 'Add Link')}</DropdownToggle>
+      <DropdownToggle className="link-picker__menu-toggle font-icon-plus-1" caret>{i18n._t('LinkField.ADD_LINK', 'Add Link')}</DropdownToggle>


Now that we are rendering links below the picker, it makes way more sense to have a + symbol for the picker, and a link symbol for the links. It's a super quick visual distinction.

GuySartorelli · 2023-11-29T01:49:06Z

README.md

-### Silverstripe 5
-
 ```sh
 composer require silverstripe/linkfield
 ```

-### GraphQL v4 - Silverstripe 4
-
-`composer require silverstripe/linkfield:^2`
-
-### GraphQL v3 - Silverstripe 4
-
-```sh
-composer require silverstripe/linkfield:^1
-```


This branch is only valid for CMS 5

GuySartorelli · 2023-11-29T01:49:38Z

README.md

-## Migrating from Version `1.0.0` or `dev-master`
-
-Please be aware that in early versions of this module (and in untagged `dev-master`) there were no table names defined
-for our `Link` classes. These have now all been defined, which may mean that you need to rename your old tables, or
-migrate the data across.
-
-EG: `SilverStripe_LinkField_Models_Link` needs to be migrated to `LinkField_Link`.


These are old and don't apply to the jump from 2.x/3.x to 4.x

README.md

client/src/components/LinkField/LinkField.js

emteknetnz · 2023-11-29T22:07:43Z

client/src/components/LinkField/LinkField.js

  const [data, setData] = useState({});
-  const [editing, setEditing] = useState(false);
+  const [editingID, setEditingID] = useState(null);


Suggested change

const [editingID, setEditingID] = useState(null);

const [editingID, setEditingID] = useState(-1);

Are we using null because 0 is reserved for editing new unsaved Links?

-1 is better than null because it's easy to make errors by testing the truthiness of editingID when 0 is a valid value. Instead explicitly test for !== -1 similar to indexOf().

We're using null because it's unset, and we can easily just do a truthy check against it. If there's an ID, it'll be truthy. I'll change to 0 if you want but -1 is nonsensical and isn't what we use anywhere to denote "not an ID".

OK 0 makes sense

client/src/components/LinkField/LinkField.js

src/Controllers/LinkFieldController.php

src/Form/MultiLinkField.php

emteknetnz

The regular singlelink field wasn't working when I tested with the latest changes, though the multilink field was working

emteknetnz · 2023-11-30T20:54:22Z

client/src/components/LinkField/LinkField.js

  const [data, setData] = useState({});
-  const [editing, setEditing] = useState(false);
+  const [editingID, setEditingID] = useState(null);


OK 0 makes sense

emteknetnz · 2023-11-30T21:05:30Z

client/src/components/LinkField/LinkField.js

+      for (const linkID of linkIDs) {
+        query.push(`itemID[]=${linkID}`);
+      }
+      const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}/?${query.join('&')}`;


Suggested change

const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}/?${query.join('&')}`;

const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}?${query.join('&')}`;

Don't need the trailing slash since there's no params being used on the endpoint

Changes
/admin/linkfield/data/?itemID[]=6&itemID[]=10&itemID[]=11&itemID[]=12
to
/admin/linkfield/data?itemID[]=6&itemID[]=10&itemID[]=11&itemID[]=12

Removed to avoid ping pong, though it doesn't affect anything one way or another.

emteknetnz · 2023-11-30T21:05:36Z

client/src/components/LinkField/LinkField.js

+    if (!editingID && linkIDs.length > 0) {
+      const query = [];
+      for (const linkID of linkIDs) {
+        query.push(`itemID[]=${linkID}`);


Suggested change

query.push(`itemID[]=${linkID}`);

query.push(`itemIDs[]=${linkID}`);

Because we don't have behat tests yet small unnecessary changes like this are more likely to break things than to be useful. I don't want to keep making these small changes that don't actually affect anything - it means I have to test every bit of functionality every time it goes through a review.

Done.

emteknetnz · 2023-11-30T21:06:13Z

src/Controllers/LinkFieldController.php

+    private function itemIDsFromRequest(): array
+    {
+        $request = $this->getRequest();
+        $itemIDs = $request->getVar('itemID');


Suggested change

$itemIDs = $request->getVar('itemID');

$itemIDs = $request->getVar('itemIDs');

GuySartorelli · 2023-11-30T21:56:18Z

The regular singlelink field wasn't working when I tested with the latest changes, though the multilink field was working

It's working for me locally with this set of changes - hopefully something I did fixed whatever was broken, but if there's still something about it that isn't working please describe the actual faulty behaviour so I know what specifically needs fixing.

emteknetnz

Tested locally, worked good

ENH Refactor LinkField in preparation for MultiLinkField

acb9c84

GuySartorelli self-assigned this Nov 28, 2023

This was referenced Nov 28, 2023

Multiple link support #107

Closed

Add multi link support #45

Closed

GuySartorelli commented Nov 28, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/multi-link-field branch from 14c1d4e to f9925f3 Compare November 28, 2023 22:13

GuySartorelli commented Nov 28, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/multi-link-field branch from f9925f3 to 8852c98 Compare November 28, 2023 23:34

GuySartorelli commented Nov 28, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/multi-link-field branch from 8852c98 to e95916b Compare November 29, 2023 00:09

GuySartorelli commented Nov 29, 2023

View reviewed changes

emteknetnz requested changes Nov 29, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/multi-link-field branch 2 times, most recently from bef7985 to 0706a98 Compare November 29, 2023 23:24

emteknetnz requested changes Nov 30, 2023

View reviewed changes

GuySartorelli added 2 commits December 1, 2023 10:55

NEW Add MultiLinkField for managing many-type relations

1be5f0d

DOC Update README

cc21f8a

GuySartorelli force-pushed the pulls/4/multi-link-field branch from 0706a98 to cc21f8a Compare November 30, 2023 21:55

emteknetnz approved these changes Nov 30, 2023

View reviewed changes

emteknetnz merged commit 9debde6 into silverstripe:4 Nov 30, 2023
10 checks passed

emteknetnz deleted the pulls/4/multi-link-field branch November 30, 2023 22:27

GuySartorelli mentioned this pull request Dec 19, 2023

ENH Tidy up permissions #141

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Add MultiLinkField #120

NEW Add MultiLinkField #120

GuySartorelli commented Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 28, 2023

GuySartorelli Nov 28, 2023

GuySartorelli Nov 28, 2023

GuySartorelli Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 28, 2023

GuySartorelli Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 28, 2023

GuySartorelli Nov 28, 2023

GuySartorelli Nov 29, 2023

GuySartorelli Nov 29, 2023

GuySartorelli Nov 29, 2023

GuySartorelli Nov 29, 2023

emteknetnz Nov 29, 2023

GuySartorelli Nov 29, 2023

emteknetnz Nov 30, 2023

GuySartorelli Nov 30, 2023

emteknetnz left a comment

emteknetnz Nov 30, 2023

emteknetnz Nov 30, 2023

GuySartorelli Nov 30, 2023

emteknetnz Nov 30, 2023

GuySartorelli Nov 30, 2023 •

edited

Loading

emteknetnz Nov 30, 2023

GuySartorelli Nov 30, 2023

GuySartorelli commented Nov 30, 2023

emteknetnz left a comment

		value: PropTypes.number.isRequired,
		value: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.number), PropTypes.number]),

		<DropdownToggle className="link-picker__menu-toggle font-icon-link" caret>{i18n._t('LinkField.ADD_LINK', 'Add Link')}</DropdownToggle>
		<DropdownToggle className="link-picker__menu-toggle font-icon-plus-1" caret>{i18n._t('LinkField.ADD_LINK', 'Add Link')}</DropdownToggle>

	const [editingID, setEditingID] = useState(null);
	const [editingID, setEditingID] = useState(-1);

	const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}/?${query.join('&')}`;
	const endpoint = `${Config.getSection(section).form.linkForm.dataUrl}?${query.join('&')}`;

	query.push(`itemID[]=${linkID}`);
	query.push(`itemIDs[]=${linkID}`);

	$itemIDs = $request->getVar('itemID');
	$itemIDs = $request->getVar('itemIDs');

NEW Add MultiLinkField #120

NEW Add MultiLinkField #120

Conversation

GuySartorelli commented Nov 28, 2023 • edited Loading

Notes

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Nov 28, 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

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

emteknetnz 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

GuySartorelli Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Nov 30, 2023

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 28, 2023 •

edited

Loading

GuySartorelli Nov 30, 2023 •

edited

Loading