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

Support Htmlable record titles #6652

Merged

Conversation

bernhardh
Copy link
Contributor

@bernhardh bernhardh commented May 31, 2023

TLDR: This PR only adds HtmlString as a return type to the getTitle and getRecordTitle method.

Reason:

I have a hotels resource, where some of the hotels have HTML inside their name, like Hotel XY <sup>Superior</sup> (Hotel XY Superior). On edit pages and inside the breadcrumb, this looks obviously uggly, since its rendered htmlencoded. After PR I can:

  public static function getRecordTitle(?Model $record): string | HtmlString | null
  {
        return new HtmlString($record->name);
  }

@what-the-diff
Copy link
Contributor

what-the-diff bot commented May 31, 2023

PR Summary

  • Changed return types for getTitle and getRecordTitle
    The return type is now more flexible, allowing both strings and HTML-formatted content in Page.php, InteractsWithRecord.php, and EditRecord.php.

  • Added new 'evaluate' method
    A new method that returns HTML-formatted content or null has been added. This improves the flexibility and output of methods in HasHelperText, HasHint, and HasDescription traits.

@zepfietje
Copy link
Member

zepfietje commented May 31, 2023

@danharrin, shouldn't all HtmlString type declarations be updated to Htmlable (also in rest of codebase)?

@zepfietje zepfietje added the enhancement New feature or request label May 31, 2023
@zepfietje zepfietje changed the title CHG: Allow record titles to be HtmlString Support Htmlable record titles May 31, 2023
@zepfietje zepfietje added this to the v2 milestone May 31, 2023
@danharrin
Copy link
Member

Correct, yes

@zepfietje
Copy link
Member

Correct, yes

Can we do this in v2 or would it be breaking somehow?
There's just a couple that still need changing.

@bernhardh
Copy link
Contributor Author

Should I change it to Htmlable in my PR?

@danharrin
Copy link
Member

We can probably change all the HtmlString types to Htmlable without breaking in v2, yes

@zepfietje
Copy link
Member

zepfietje commented May 31, 2023

Could you handle this and mark as ready for review when done, @bernhardh? 😊

@zepfietje zepfietje marked this pull request as draft May 31, 2023 10:57
… into feature/allow-htmlstring-titles

# Conflicts:
#	packages/admin/src/Pages/Page.php
#	packages/admin/src/Resources/Pages/Concerns/InteractsWithRecord.php
#	packages/admin/src/Resources/Resource.php
@bernhardh
Copy link
Contributor Author

bernhardh commented Jun 1, 2023

@zepfietje Done and thank you both for your great work! How to mark it as ready for review? I can't add labels to issues?

@zepfietje zepfietje marked this pull request as ready for review June 1, 2023 08:12
@zepfietje
Copy link
Member

Thanks, @bernhardh!
LGTM, handing over to @danharrin for merge.

How to mark it as ready for review?

For drafts there's a button at the bottom of the PR where you'd usually have the merge button. I've marked it as ready now. 👍

@danharrin danharrin merged commit 95670d5 into filamentphp:2.x Jun 1, 2023
@juliomotol
Copy link
Contributor

juliomotol commented Jun 26, 2023

Not a breaking change but ever since we upgraded our Filament version, we encountered this in PHPStan with level 8:

Binary operation "." between Illuminate\Contracts\Support\Htmlable|string and ' ' results in an error.

This is where we try to contcat getTitle() with another string.

@zepfietje
Copy link
Member

Maybe cast it to a string first, @juliomotol?

@juliomotol
Copy link
Contributor

Tried that, and it says:

Cannot cast Illuminate\Contracts\Support\Htmlable|string to string.

@zepfietje
Copy link
Member

Right, then you could check if it's Htmlable and call toHtml().

@juliomotol
Copy link
Contributor

To add more context in this situation, we're not concatenating on getTitle() for display but rather for it to be saved to a database (more specifically to generate a description for an activity log). For now, we'll omit using getTitle() and use our own logic instead.

@bernhardh
Copy link
Contributor Author

@zepfietje wheren't these changes merged to v3? I am trying to update to v3, but they are once again not there. For example packages/admin/src/Resources/Pages/Concerns/InteractsWithRecord.php is again just returning string instead of Htmlable|string

@zepfietje
Copy link
Member

I think so. CC @danharrin.

@bernhardh
Copy link
Contributor Author

bernhardh commented Aug 11, 2023

Ah, forget it. I am talking about the breadcrumb and this didn't work in v2 either ;).

V2

image

V3

image

Will have a closer look and then maybe submit a new PR

@bernhardh bernhardh mentioned this pull request Aug 23, 2023
3 tasks
@bernhardh bernhardh deleted the feature/allow-htmlstring-titles branch December 4, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants