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.

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

ENH Rename and move Title field #207

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Feb 7, 2024

I fixed a few issues at once because it made sense because they're very closely related

The general approach of this PR was to update PHP change 'Title' to 'LinkText', though JS was left completely alone and still uses 'title'

Also in the XHR request to get linkData, there is now always a populated 'Title' field returned that returns getTitle().

This means we'll never have a empty title field unless someone puts in custom code to have a blank default title in which case it'll still have the unclickable area, though that's such an edge case I don't think we need to worry about it,

Issues

* @return void
* @dataProvider linkUrlCasesDataProvider
*/
public function testGetUrl(string $identifier, string $class, string $expected): void
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated refactoring to move the data provider above the unit test to match the rest of the file

@@ -241,7 +227,21 @@ public function linkUrlCasesDataProvider(): array
];
}

function linkDefaultTitleDataProvider(): array
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated refactoring to rename this to provideGetUrl() to match our conventions

@@ -303,7 +303,7 @@ function linkDefaultTitleDataProvider(): array
}

/**
* @dataProvider linkDefaultTitleDataProvider
* @dataProvider provideDefaultLinkTitle
Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated refactoring to get this to match our naming conventions

@emteknetnz emteknetnz marked this pull request as ready for review February 7, 2024 23:56
LINK_TYPE_TITLE: 'Link Type'
MISSING_DEFAULT_TITLE: 'No link provided'
MISSING_DEFAULT_TITLE: '(No value provided)'
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're now always showing a title, this was the default provided for custom links without LinkText - I've updated it to something more sensible since it's more likely the custom link have a "value" rather that a "link (to a relation)"

@@ -30,26 +30,26 @@ en:
FILE_DOES_NOT_EXIST: 'File does not exist'
FILE_FIELD: File
LINKLABEL: 'Link to a file'
MISSING_DEFAULT_TITLE: 'File missing'
MISSING_DEFAULT_TITLE: '(File missing)'
Copy link
Member Author

@emteknetnz emteknetnz Feb 8, 2024

Choose a reason for hiding this comment

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

This is unrelated refactoring to add brackets to "File missing" and "Page missing" default titles so that it matches "(No value provided)" for generic links

It's just a little something to make it really obvious that it's not regular link text

@@ -81,10 +81,10 @@ public function testLinkFormGetSchema(
. "&ownerRelation=$ownerRelation";
$this->assertSame($expectedAction, $formSchema['schema']['action']);
// schema is nested and retains 'Root' and 'Main' tab hierarchy
$this->assertSame('Phone', $formSchema['schema']['fields'][0]['children'][0]['children'][1]['name']);
$this->assertSame('Phone', $formSchema['schema']['fields'][0]['children'][0]['children'][0]['name']);
Copy link
Member Author

Choose a reason for hiding this comment

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

These array index changes are required because we're move LinkText to the bottom in getCMSFields()

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Most of the way there, just a few things to tidy up

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Models/SiteTreeLink.php Outdated Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works well locally.

@GuySartorelli GuySartorelli merged commit 3786a07 into silverstripe:4 Feb 8, 2024
10 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4/title-field branch February 8, 2024 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants