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

DOC Document code moved from cms to framework #625

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

GuySartorelli
Copy link
Member

Copy link
Member Author

Choose a reason for hiding this comment

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

Note the rest of the docs on this page will be retouched in silverstripe/silverstripe-cms#2951


- The `SiteTree.description` configuration property has been renamed to [`class_description`](api:SilverStripe\ORM\DataObject->class_description).
This configuration has been added to `DataObject` along with the corresponding [`classDescription()`](api:SilverStripe\ORM\DataObject::classDescription()) and [`i18n_classDescription()`](api:SilverStripe\ORM\DataObject::i18n_classDescription()) methods.
- The [`Hierarchy`](api:SilverStripe\ORM\Hierarchy\Hierarchy) extension now has a bunch of configuration and methods which used to be exclusive to `SiteTree`.
Copy link
Member Author

Choose a reason for hiding this comment

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

Part of silverstripe/silverstripe-cms#2951 will be updating the CMS Tree docs which will explain more about Hierarchy and how it's used - when that happens this note will link to that page.

Copy link
Member

Choose a reason for hiding this comment

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

This note will link to that page? I'm not sure what this means? Does that mean you need to add something else to this PR?

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 26, 2024

Choose a reason for hiding this comment

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

When silverstripe/silverstripe-cms#2951 is done, it will include new documentation about how Hierarchy works. I will be adding a link to that new page to this bullet point (something like "see for more details")

Nothing to add for this PR. It's just a note about why this bullet point doesn't currently link away to more details (because there isn't appropriate docs for that right now) and seems a bit vague on its own.

en/08_Changelogs/6.0.0.md Outdated Show resolved Hide resolved
en/08_Changelogs/6.0.0.md Show resolved Hide resolved
@GuySartorelli GuySartorelli marked this pull request as ready for review November 14, 2024 04:40
en/08_Changelogs/6.0.0.md Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

Yup, forgot to update this. The whole "actually using it" thing was gonna be done in minors originally. Will update this now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not used anymore in favour of the new screenshots

Copy link
Member

Choose a reason for hiding this comment

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

status-flag.png shouldn't include the fluent icons since most install won't have those, so this may confuse people why their site does not have them

status-flags-custom.png has misaligned text in the badges

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 26, 2024

Choose a reason for hiding this comment

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

status-flag.png shouldn't include the fluent icons since most install won't have those, so this may confuse people why their site does not have them

I've explicitly mentioned fluent as a thing that can add status flags, and pointed out that those status flags specifically come from there. Fluent is a supported module. I don't see anything wrong with including them in this screenshot and I don't think it's a valuable use of my time to retake this one.

Having it there also provides an example of multiple status flags in the breadcrumbs in CMSMain which would otherwise not be shown. I don't want to add a custom one in this screenshot - that's what status-flags-custom.png is for.

Copy link
Member Author

Choose a reason for hiding this comment

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

status-flags-custom.png has misaligned text in the badges

It's a screenshot of how it looks.

Copy link
Member

Choose a reason for hiding this comment

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

status-flag.png shouldn't include the fluent icons since most install won't have those, so this may confuse people why their site does not have them

status-flags-custom.png has misaligned text in the badges


This is already used to show what locale the data is in for models localised using [`tractorcow/silverstripe-fluent`](https://github.com/tractorcow-farm/silverstripe-fluent/), and what versioned stage it's in for models using the [`Versioned`](api:SilverStripe\Versioned\Versioned) extension.

Status flags are displayed in breadcrumbs at the top of edit forms in the CMS, in the site tree for `CMSMain`, for each row in a grid field, and in [``]() and [``]().
Copy link
Member

Choose a reason for hiding this comment

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

and in ]() and [

Looks like you've forgotten to fill in some text and links here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, will fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated


- The `SiteTree.description` configuration property has been renamed to [`class_description`](api:SilverStripe\ORM\DataObject->class_description).
This configuration has been added to `DataObject` along with the corresponding [`classDescription()`](api:SilverStripe\ORM\DataObject::classDescription()) and [`i18n_classDescription()`](api:SilverStripe\ORM\DataObject::i18n_classDescription()) methods.
- The [`Hierarchy`](api:SilverStripe\ORM\Hierarchy\Hierarchy) extension now has a bunch of configuration and methods which used to be exclusive to `SiteTree`.
Copy link
Member

Choose a reason for hiding this comment

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

This note will link to that page? I'm not sure what this means? Does that mean you need to add something else to this PR?

Comment on lines 39 to 47
private static array $db = [
'IsPublishScheduled' => 'Boolean',
];

public function getStatusFlags(bool $cached = true): array
{
$this->beforeExtending('updateStatusFlags', function (array &$flags) {
if ($this->IsPublishScheduled) {
$flags['scheduledtopublish'] = _t(__CLASS__ . 'SCHEDULE_PUBLISH', 'Scheduled To publish');
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
private static array $db = [
'IsPublishScheduled' => 'Boolean',
];
public function getStatusFlags(bool $cached = true): array
{
$this->beforeExtending('updateStatusFlags', function (array &$flags) {
if ($this->IsPublishScheduled) {
$flags['scheduledtopublish'] = _t(__CLASS__ . 'SCHEDULE_PUBLISH', 'Scheduled To publish');
private static array $db = [
'MyColour' => 'Varchar',
];
public function getStatusFlags(bool $cached = true): array
{
$this->beforeExtending('updateStatusFlags', function (array &$flags) {
if (in_array($this->MyColour, ['Red', 'Green'])) {
$flags['mycolour'] = $this->MyColour;

I think it's better to just use arbitary example data rather than using "IsPublishScheduled" which kind of implies there's "scheduled publishing" functionality somewhere in the CMS, and I think we should just avoid "publishing" in general here as that's related to Versioning, and the point of this is we're abstracting out something that's traditionally been there mostly just there to provide versioned status badges

Also I don't think including the _t() function here is helpful as that's another concept all together, and I think the example should only be demonstrating the primary things it's trying to demonstrate

The simpler the example is the easier it is to follow

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed in person since the problem you had was the word "published" because it's similar to versioning concepts I've just reworked it to avoid that word while still giving a meaningful example that hints at the sorts of use cases this might have.

I think using _t() in examples promotes best practice but as discussed will leave it out for the sake of getting this merged.

I've also updated the screenshot so it reflects the custom flag from the example.

You can include additional context which will be displayed when hovering over your status flags. This is done using an associative array where the main text of the flag is in the `text` key, and the additional information is in the `title` key:

```php
namespace App\Model;
Copy link
Member

Choose a reason for hiding this comment

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

I think this example should be changed like I've recommended above i.e. just use "MyColour" and don't use _t()

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

@emteknetnz emteknetnz merged commit edf38cd into silverstripe:6 Nov 26, 2024
3 checks passed
@emteknetnz emteknetnz deleted the pulls/6/move-from-cms branch November 26, 2024 06:21
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