-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
Inline version of KirbyText #1442
Conversation
3f4d907
to
6d45fa4
Compare
2a7b830
to
cced621
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional param is a breaking change as the $data
param now moves to third position. I think we should either create a separate method and helper for this behavior or make the params "magic" (so that the order of the additional params doesn't matter).
Also I think we could use Parsedown
's line()
method instead of stripping the paragraph tags manually. This should be supported down to the markdown
component.
362ad92
to
9d3fe66
Compare
cced621
to
c5bfff8
Compare
I agree with @lukasbestle. |
Understood and agreed - can't say yet when I will get to rewrite it though. |
0043c18
to
3481797
Compare
c5bfff8
to
cff410e
Compare
@lukasbestle I am wondering whether it is worth the effort to make the parameters "magic". Or if it would be sufficient to move the new parameter to the last position to not create a breaking change. What do you think? |
You are right about that. It's probably even harmful as it gets a lot more complex. I think we should avoid methods with a lot of params in general. We had that in v2 in some places (e.g. the So what about a completely separate method for this? |
Yes, call it |
Ok 👍 separate method it is. And then I will try to solve it at the markdown component level |
I'm not sure about the Maybe |
Hmm yea, I feel like adding anything, even just 1-2 characters to |
I hardly ever use |
I don't think we should introduce an entirely new term for this, that could be quite confusing. @texnixe Do we already have a short |
If you have a long |
👍 for |
cff410e
to
97b0049
Compare
@lukasbestle ping - new version. I still don't like the name to be honest 🙈 |
Needs a bit of adaption if this PR gets merged: #1543 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also have inline variants of the markdown
helper and field method. Not sure about that though.
Otherwise LGTM. 👍
config/helpers.php
Outdated
* @param array $data | ||
* @return string | ||
*/ | ||
function kirbytextinline(string $text = null, array $data = []): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use camelCase
for this: kirbytextInline
. Same for all other occurrences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait to hear from @bastianallgeier if all else would be ok, before making that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept it like the rest. Otherwise we would need to change kirbyText, kirbyTags, etc. as well.
97b0049
to
5112640
Compare
Describe the PR
Adds option for KirbyText to output unwrapped (without wrapping
<p>
tag) string.kirbytext($text, false)
->kirbytext(false)
Related issues
$page->field()->kt(false)
forkirbytextRaw
ideas#93Todos
composer fix