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

Jump to bottom

NEW Allow developers to exclude LinkText field #215

Merged
merged 1 commit into from
Feb 12, 2024
Merged

NEW Allow developers to exclude LinkText field #215

merged 1 commit into from
Feb 12, 2024

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Feb 12, 2024 •
edited
Loading

Because this gets passed from PHP through to JS, back to PHP for the controller, and then finally through to JS again for the form modal, I think the only way to adequately test this is through behat tests. There's a card in progress right now to create behat tests, so I'm not going to duplicate efforts creating a behat setup in this PR.

If the reviewer agrees, I'll create a separate card to add behat tests for this scenario, which can be blocked on the existing behat card.

Issue

@GuySartorelli GuySartorelli mentioned this pull request Feb 12, 2024
Closed
GuySartorelli
GuySartorelli commented Feb 12, 2024
View reviewed changes
src/Form/Traits/AllowedLinkClassesTrait.php
Comment on lines +18 to +29
private bool $excludeLinkTextField = false;

public function setExcludeLinkTextField(bool $include): static
{
$this->excludeLinkTextField = $include;
return $this;
}

public function getExcludeLinkTextField(): bool
{
return $this->excludeLinkTextField;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to create a whole separate trait just for this, nor did I want to duplicate code by putting it into each field.

I've added a note to #78 to consider combining the traits into an abstract class instead - assuming that is done, it doesn't matter too much where this code goes for this PR since it'll all end up in the same place anyway.

emteknetnz
emteknetnz requested changes Feb 12, 2024
View reviewed changes
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Add a unit test on LinkFieldController that passing an 'excludeLinkTextField' get param will result in a schema without a LinkText field

src/Controllers/LinkFieldController.php Outdated
@@ -88,7 +88,7 @@ public function linkForm(): Form
}
$operation = 'create';
}
return $this->createLinkForm($link, $operation);
return $this->createLinkForm($link, $operation, (bool) $this->getRequest()->getVar('excludeTextField'));
Copy link
Member

Choose a reason for hiding this comment

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

Bit messy having this one on line, create an intermediate variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd argue having the intermediate variable is unnecessary 😅 Gotta love subjective code styles.
I've made the change.

client/src/components/LinkField/LinkField.js Outdated
@@ -52,6 +52,7 @@ const LinkField = ({
ownerID,
ownerClass,
ownerRelation,
excludeTextField = false,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
excludeTextField = false,
excludeLinkTextField = false,

It's really confusing omitting the Link from this variable, especially since in other places it's not omitted. It's a specific field we want to remove, not just a generic 'textfield'

Update all variables through including the ones passed to jquery as an element attribute e.g. 'data-exclude-textfield' should be 'data-exclude-linktext-field

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@GuySartorelli
Copy link
Member Author

Add a unit test on LinkFieldController that passing an 'excludeLinkTextField' get param will result in a schema without a LinkText field

Done

GuySartorelli
GuySartorelli commented Feb 12, 2024
View reviewed changes
tests/php/Controllers/LinkFieldControllerTest.php
Comment on lines -5 to -8
use SilverStripe\CMS\Model\SiteTree;
use ReflectionMethod;
use SilverStripe\Dev\FunctionalTest;
use SilverStripe\LinkField\Tests\Controllers\LinkFieldControllerTest\TestPhoneLink;
use SilverStripe\Core\Config\Config;
Copy link
Member Author

Choose a reason for hiding this comment

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

use statements that were removed are unrelated refactoring - those were not in use.

emteknetnz
emteknetnz requested changes Feb 12, 2024
View reviewed changes
client/src/entwine/LinkField.js Outdated
@@ -51,6 +51,7 @@ jQuery.entwine('ss', ($) => {
ownerID: inputField.data('owner-id'),
ownerClass: inputField.data('owner-class'),
ownerRelation: inputField.data('owner-relation'),
excludeLinkTextField: inputField.data('exclude-textfield'),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
excludeLinkTextField: inputField.data('exclude-textfield'),
excludeLinkTextField: inputField.data('exclude-linktext-field'),

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah oops. Fixed.

src/Form/LinkField.php Outdated
@@ -51,6 +51,7 @@ protected function getDefaultAttributes(): array
$attributes['data-owner-id'] = $ownerFields['ID'];
$attributes['data-owner-class'] = $ownerFields['Class'];
$attributes['data-owner-relation'] = $ownerFields['Relation'];
$attributes['data-exclude-textfield'] = $this->getExcludeLinkTextField();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$attributes['data-exclude-textfield'] = $this->getExcludeLinkTextField();
$attributes['data-exclude-linktext-field'] = $this->getExcludeLinkTextField();

src/Form/MultiLinkField.php Outdated
@@ -70,6 +71,7 @@ protected function getDefaultAttributes(): array
$attributes['data-owner-id'] = $ownerFields['ID'];
$attributes['data-owner-class'] = $ownerFields['Class'];
$attributes['data-owner-relation'] = $ownerFields['Relation'];
$attributes['data-exclude-textfield'] = $this->getExcludeLinkTextField();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$attributes['data-exclude-textfield'] = $this->getExcludeLinkTextField();
$attributes['data-exclude-linktext-field'] = $this->getExcludeLinkTextField();

emteknetnz
emteknetnz approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Tested locally works good

@emteknetnz emteknetnz merged commit 04ae65c into silverstripe:4 Feb 12, 2024
10 checks passed
@emteknetnz emteknetnz deleted the pulls/4/no-link-text branch February 12, 2024 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emteknetnz emteknetnz emteknetnz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GuySartorelli @emteknetnz