Conversation
davisagli
left a comment
davisagli
left a comment
There was a problem hiding this comment.
Does this still work if the site does NOT use a version of VLT that includes the soft_text_widget? Or is it a breaking change?
|
@davisagli it still work because default case handle it and it is same as previous version. |
There was a problem hiding this comment.
LGTM, relies on kitconcept/volto-light-theme#747
It will need the camelCase adjustments.
|
@davisagli I can't recall the full reasoning behind, but we decided to not shadow the default Volto's TextWidget, for maintenance reasons, thus using the custom widget. Another alternative is to enhance Volto's TextWidget, but initially it seemed too dirty to me. Which is your take on this? |
davisagli
left a comment
davisagli
left a comment
There was a problem hiding this comment.
I tested this with DLR, which uses kitconcept.seo but not VLT. It falls back to the default widget, so that part looks good to me.
You need to update the translations for the text that you changed.
I can't recall the full reasoning behind, but we decided to not shadow the default Volto's TextWidget, for maintenance reasons, thus using the custom widget. Another alternative is to enhance Volto's TextWidget, but initially it seemed too dirty to me. Which is your take on this?
@sneridagh It would be a nice feature to have in the standard text widget, wouldn't it? But if that's too much effort, I'm okay with the custom widget.
|
@davisagli let's talk about it this evening. |
|
@sneridagh Like I said, I don't have a strong opinion about whether to put it in Volto or VLT. |
|
@sneridagh please merge this and release since vlt pr got merged.kitconcept/volto-light-theme#747 cc @davisagli |
|
Released VLT 8a13: |
|
@davisagli please can you make a release? Also, there's another PR open that you might want to merge. |
|
@sneridagh @ericof releaesed 2.1.2, and I asked him to review my PR |
| @@ -0,0 +1 @@ | |||
| Add new widget for seo title and description. @iFlameing No newline at end of file | |||
There was a problem hiding this comment.
@iFlameing @sneridagh this changelog entry says "Add new". How come this is a bugfix and has been released as a bugfix release? If we manage our releases that way, we will piss off our devs who upgrade and our clients who see new features without a heads-up.
There was a problem hiding this comment.
Just saw that this requires a new version of VLT. The feature alone might be worth a major version bump. I honestly don't get how so many people can check and discuss this PR an no one sees this. cc @ericof
There was a problem hiding this comment.
Sorry for the tone folks. Had a bad morning...
There was a problem hiding this comment.
@tisto you can read the discussion in the PR where I pointed this out and carefully tested to make sure the add-on still works (without the new widget) with older versions of VLT. It is not breaking.
You're right that it should have been a minor release and marked as a feature though.
5 participants
This PR adds a new widget for the title and description, developed in coordination with VLT. When softMaxLength is passed, a yellow label is shown that hints at how much text you are exceeding as you start typing.
screenshot