Skip to content

Update TextCheckboxGroupField to use natural TitleShowTitle field naming#39

Merged
jsirish merged 2 commits into5from
feature/natural-titleshowtitle-field-naming
Sep 3, 2025
Merged

Update TextCheckboxGroupField to use natural TitleShowTitle field naming#39
jsirish merged 2 commits into5from
feature/natural-titleshowtitle-field-naming

Conversation

@jsirish
Copy link
Copy Markdown
Member

@jsirish jsirish commented Sep 3, 2025

Summary

Updates BaseElementObject to use natural TitleShowTitle field naming by modifying the TextCheckboxGroupField creation approach.

Changes

  • Modified: TextCheckboxGroupField::create('Title') instead of ->setName('Title')
  • Result: Field name becomes TitleShowTitle naturally (SilverStripe framework behavior)
  • Consistency: Matches pattern across all BaseElementObject descendants

Technical Details

Before

TextCheckboxGroupField::create()
    ->setName('Title')
    ->setTitle($this->fieldLabel('Title'))

After

TextCheckboxGroupField::create('Title')
    ->setTitle($this->fieldLabel('Title'))

Integration Context

This change is part of a coordinated update with CustomStylesExtension in essentials-tools:

  1. 📋 essentials-tools: Updated CustomStylesExtension to position fields using TitleShowTitle for BaseElementObject descendants (PR #46)
  2. 📋 baseobject (this PR): Updated field creation to generate TitleShowTitle naturally

Note: After investigation, carousel's Slide class was already using the correct TextCheckboxGroupField::create('Title') pattern, so no changes were needed there.

Backwards Compatibility

  • Impact: Changes field name from Title to TitleShowTitle for BaseElementObject descendants
  • Timeline: Affects projects from the last 12 months
  • Assessment: Manageable impact due to recent introduction
  • Mitigation: Clear documentation and coordinated rollout

Testing

  • Field name generation verified through SilverStripe framework behavior
  • Integration tested with CustomStylesExtension positioning logic
  • Consistent behavior across all BaseElementObject descendants

Related Work

- Use TextCheckboxGroupField::create('Title') instead of ->setName('Title')
- This creates 'TitleShowTitle' field name naturally
- Ensures consistency with CustomStylesExtension positioning logic
- Part of unified approach across BaseElementObject descendants
Copilot AI review requested due to automatic review settings September 3, 2025 17:08
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the BaseElementObject class to use natural field naming for the TextCheckboxGroupField by passing the field name directly to the create() method instead of using setName(). This change ensures the field name becomes TitleShowTitle naturally following SilverStripe framework behavior.

  • Simplified field creation by passing field name to constructor
  • Enables natural TitleShowTitle field naming for consistency with framework patterns
  • Part of coordinated update across multiple repositories for field positioning

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@jsirish
Copy link
Copy Markdown
Member Author

jsirish commented Sep 3, 2025

@muskie9, this is in relation to https://github.com/dynamic/silverstripe-essentials-tools/pull/46

Tagged you on the review in case you can think of scenarios where this would be a breaking change

@muskie9
Copy link
Copy Markdown
Member

muskie9 commented Sep 3, 2025

@jsirish It looks like v5 of this module may be used by some of our other newer sites so we may need to check how the fields are affected as those projects are updated over time.

@jsirish jsirish merged commit 6763958 into 5 Sep 3, 2025
18 checks passed
@jsirish jsirish deleted the feature/natural-titleshowtitle-field-naming branch September 3, 2025 19:26
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.

3 participants