Several enhancements & new features#5
Conversation
… with 'meta_' to avoid excessive loop
|
@8blvck thanks for the contribution! Couple of notes:
Does your adjustment to the metadata storage allow for it to be stored inside of already existing JSON blob columns alongside pre-existing data? |
Are you talking about prefixing properties in JSON with "meta_"? Then I believe so, since it stored in {
"seo": {
"meta_image":"",
"meta_title":"Winter Blog Post Title",
"meta_description":"Winter Blog Post Description",
"meta_nofollow":"1"
}
}UPD: |
Plugin.php
Outdated
| ], | ||
| \Winter\Blog\Controllers\Categories::class => [ | ||
| \Winter\Blog\Models\Category::class, | ||
| ], |
There was a problem hiding this comment.
Does it make sense to make this configurable in the config file so that attaching SEO fields to custom models would be as easy as adding them to the config?
There was a problem hiding this comment.
Something similar to https://github.com/LukeTowers/wn-easyaudit-plugin/blob/main/config/config.php#L68 & https://github.com/LukeTowers/wn-easyaudit-plugin/blob/main/Plugin.php#L225.
If we added a model behaviour (Seoable or something) that also provided helper methods for getting / setting the SEO values then we could use that example pretty much verbatim which would simplify the below logic a lot.
| } | ||
| win.ready = ready; | ||
| win.counter = counter; | ||
| win.ready('[data-counter]', (el) => { |
There was a problem hiding this comment.
It might be better to look at rewriting this as a Snowboard.js plugin, see https://github.com/wintercms/winter/blob/develop/modules/backend/formwidgets/iconpicker/assets/js/src/iconpicker.js as an example, specifically the last two lines which enable the use of data-control="iconpicker" to automatically attach the JS to the relevant elements.
There was a problem hiding this comment.
Hmm, on second thought with the goal of attaching this to fields using the attributes property on fields this is probably fine as it is, although the indentation still needs to be fixed here.
There was a problem hiding this comment.
@bennothommo on third thought what do you think about adding support for character counting functionality into the Winter core? It's been asked for a few times and Inetis even created a plugin for it once: https://github.com/inetis-ch/oc-InputCounter-plugin
Refs:
- https://octobercms.com/forum/post/character-count
- https://octobercms.com/forum/post/character-counter-in-excerpt
- Trouble Implementing Character Count Froala Editor octobercms/october#4370
- https://octobercms.com/forum/post/plugin-builder-rich-editor-froala-limit-the-number-of-characters
- https://froala.com/wysiwyg-editor/examples/character-counter/
There was a problem hiding this comment.
I kinda feel like that would still be useful as a plugin, unless we have specific fields in core that have limits on length.
There was a problem hiding this comment.
Do you have any objections to writing it as a Snowboard plugin that's available in the core (with initial support for the text / textarea / maybe Froala fields)? That way we can use it in this plugin without having to have it only included in this plugin.
There was a problem hiding this comment.
I'll write it as a first party plugin, as I would like to create an example of a plugin that provides Snowboard assets. If you feel afterwards that it should be in core, we can move it.
There was a problem hiding this comment.
Works for me. Are you going to do it in this plugin?
There was a problem hiding this comment.
Nah, I'll do it as its own plugin, and just add it as a requirement for this plugin. You never know - there may be other plugins that could use the same feature down the line.
models/settings/fields.yaml
Outdated
| global_app_name: | ||
| tab: 'Global' | ||
| label: 'App name' | ||
| span: auto | ||
| placeholder: '' | ||
| type: text | ||
| default: '' | ||
| comment: 'App name can be visible in the title meta tag' | ||
| global_separator: | ||
| tab: 'Global' | ||
| label: 'App name separator' | ||
| span: auto | ||
| placeholder: '' | ||
| type: text | ||
| default: '|' | ||
| comment: 'Sybmol to separate title from app name: {app name} {sepratator} {title}' | ||
| global_app_name_pos: | ||
| tab: 'Global' | ||
| label: 'Select how the app name should appear in the title' | ||
| options: | ||
| hide: 'Hide' | ||
| prefix: 'Prefix' | ||
| suffix: 'Suffix' | ||
| span: auto | ||
| type: balloon-selector | ||
| global_app_title: | ||
| tab: 'Global' | ||
| label: 'App title' | ||
| size: tiny | ||
| span: full | ||
| placeholder: 'Your app global title' | ||
| type: text | ||
| comment: '[data-counter]' | ||
| attributes: | ||
| data-counter: 1 | ||
| data-min: 50 | ||
| data-max: 60 |
There was a problem hiding this comment.
This could all be simplified down into a single field that supports string replacements with a default value of {page_title} | {app_name}; with that string being parsed by the SEOTags component before rendering using the current title of the page object as {page_title} and the current app.name as the {app_name} text; and only doing this if there is not already an explicit title meta tag setup.
That way in a single field they can do any format, positioning, separator that they could possibly want without requiring all of these extra fields.
There was a problem hiding this comment.
Additionally this field should probably be called "Default Meta Title". Comment: "Meta title that is used for the page when one isn't set already. Use {page_title} to reference the page's title and {app_name} to reference this website's name".
models/settings/fields.yaml
Outdated
| enable_favicon: | ||
| tab: 'Favicon' | ||
| label: 'Enable icons' | ||
| span: full | ||
| type: switch | ||
| default: 0 | ||
| favicon: | ||
| tab: 'Favicon' | ||
| commentAbove: 'Global favicon' | ||
| type: mediafinder | ||
| mode: image | ||
| span: left |
There was a problem hiding this comment.
There's no need to have two separate settings for this, if a favicon has been uploaded or configured in the settings then it's enabled and if it's not present then it's disabled.
On a side note; it would be better to use a fileupload field here instead since you wouldn't want the favicon to be accidentally moved, removed, or used elsewhere on the site so it would be better to have the one to one relationship.
Additionally is the .ico format anything special that we need to be converting automatically beyond just resizing to 16x16 and hoping for the best?
There was a problem hiding this comment.
Something else that I've been considering for a while as well is adding support for favicons to the Winter core itself in the backend branding settings area. Ideally I'd like it to handle as much of what https://realfavicongenerator.net/ handles automatically from just uploading a single image and then what this plugin could do is pull the generated meta tags into the frontend (since they would otherwise only be injected into the backend).
That way the site's favicon management stays where it makes logical sense for it to be (with the rest of the branding settings) and it simplifies the logic that would have to be in this plugin.
Any thoughts on that?
There was a problem hiding this comment.
@LukeTowers I completely agree with you.
What I always get from RealFavicon when I make a generation:
Easy to resize:
- favicon.ico (32 X 32px)
- favicon-16x16.png
- favicon-32x32.png
- apple-touch-icon.png (180 X 180px)
- android-chrome-192x192.png
- android-chrome-384x384.png
- android-chrome-512x512.png
More complicated (?):
- mstile-150x150.png (but 270 X 270px with a 150px square image centered)
- safari-pinned-tab.svg - silhouette image
Other non images:
- browserconfig.xml (contains references to one color and to the mstile-150x150.png file)
- site.webmanifest (contains references to android images, names and colors)
It can be difficult to manage the conversion of different formats and may require the installation of a graphics library.
One possible solution is to use the Favicon API, in this case it require an extra step for the end user: getting an API key.
There was a problem hiding this comment.
I would love to use the API! @damsfx is that something you'd be willing to play around with?
There was a problem hiding this comment.
Sure!
But I'm going to wait and see how this PR evolves before I make any changes, as it modifies the plugin in a number of ways.
It might have perhaps been worth splitting it up into different functionalities.
There was a problem hiding this comment.
@damsfx honestly you could probably just create a new PR just for that functionality branching from the current main branch; I'm not sure this PR will be merged as one piece in its current state.
components/seotags/default.htm
Outdated
| {% if tagName == 'title' %} | ||
| <title>{{ tagContent }}</title> | ||
| {% else %} | ||
| <meta name="{{ tagName }}" property="{{ tagName }}" content="{{ tagContent }}" /> | ||
| {% endif %} |
There was a problem hiding this comment.
It is not always desirable to have the same <title> and , see https://stackoverflow.com/a/21076311.
It's usually the responsibility of the theme to handle the title tag (see https://github.com/wintercms/wn-workshop-theme/blob/main/partials/html/head.htm#L7). While it would be nice to make the Tab Title configurable with this plugin that wouldn't work across the board on all themes unless the theme was using <title>{{ this.page.title }}</title> in which case we could modify that immediately before rendering it after parsing it through our setting, but most themes will not be doing that.
|
Looking good @8blvck, you've got me excited to see this finished :) I've added a bunch of thoughts and guidance; let me know what you think or if you have any questions! |
|
Hey @LukeTowers, I'm pretty curious about having Seoable behavior. Currently testing plugin. everything works as expected on php 8.1. About HTML minification you absolutely right, even tho it sucks, i'd rather have this option, than not :) |
|
@LukeTowers @VadBe What remains to be done about this PR? |
|
There's still some pending comments of mine that haven't been responded to. @VadBe is this still something you'd like to push across the finish line? |
4 participants
public $metadata_from = 'column_name';, stupid and simple.Model's metadata can be set from page controller by
$this->seoTags->useMetadataModel($modelEntity);