-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
🎨 Added classes to tags helper output #22169
Conversation
no issue - Far easier to customize the tags helper output when the parts have CSS classes - Avoids the need to write custom code to customize how tags look - unless you really want to
WalkthroughThe pull request updates the tags helper functionality and its test cases. In the helper function, the import of the Handlebars-based template service has been removed and replaced with Lodash’s template function for generating HTML. The implementation now differentiates linked tags from non-linked ones by using separate functions. Additionally, prefixes, suffixes, and tag separators are now wrapped in 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (5)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Hey @brandishwar ! Welcome to the Ghost repo! :)
I don't work for Ghost.org - I'm a community contributor like you.
I think that improving the output of {{tags}} with some useful classes is a great idea. However, I think the execution needs work. My big concern: You're changing the current behavior, in a way that's absolutely going to break some themes. In particular, currently the output of {{tags}} may safely be used as a plain string, in places where HTML tags are not appropriate.
Here's what I suggest instead (but again, I'm not the core team):
- Retain the existing behavior as much as possible. If your changes require changing the results of an existing test, carefully ask why, and what might break.
- New span wrapping should be controlled by an argument. (wrapAll ?)
- There should be tests for the new functionality (set to true, not set, set to false)
It's probably safe to add a new class to existing tags in the presence of autolink, but new html is definitely going to mess something up for someone.
And one scenario where someone would want to keep the plaintext (with autolink Off) is... metadata beyond what sites are supported by default in Ghost. Probably other scenarios for it that I'm not considering. Wrapping this behind a new parameter I think is the best way to go - Off by default, of course. So I'll withdraw this PR while I get that sorted. |
Sounds like a plan! |
no issue