wintercms / wn-pages-plugin

Static Pages plugin for Winter CMS
https://wintercms.com/
MIT License
9 stars 22 forks source link

Update documentation.md #17

Closed AIC-BV closed 3 weeks ago

AIC-BV commented 1 year ago

The 'default' text does not work and would be weird to support. Searching in code wouldn't make sense because the client changed the text anyways? Also if client leaves this field empty, you will render empty HTML elements. Which makes me even think this entire feature should be removed?

BennoThommo:

I've noticed the same issue before. Currently, it won't use a default value like that, and I think the reasoning is because when you save the field in the Backend without entering anything in the field, it saves the field as an empty value. The problem is, there's no way to tell if it's intentionally empty or not - for example, if you're offering this theme to a client to modify, they may intentionally not want a tagline to be used on a page, so they would leave the field blank.

LukeTowers commented 1 year ago

I don't quite understand what you mean

bennothommo commented 1 year ago

@LukeTowers the issue is that the Syntax Parser implicitly supports default values by adding some content within the syntax tags, eg. "Our wonderful website" within text tags:

{text name="websiteName" label="Website Name"}Our wonderful website{/text}

If the variable doesn't exist or is null, it's supposed to use this default value. However, in the Pages plugin when saving empty dynamic fields, they are stored in the page as an empty string. There's no way to tell from this if the user intended to use the default value, or indeed wanted the value to be empty. As a consequence, the Pages plugin doesn't support these default values, as it assumes that an empty string is a legitimate value.

LukeTowers commented 1 year ago

@bennothommo hmm, there should be a way to make it work though because the variable itself won't exist in the file until it has been saved at least once, at which point it will be set as an empty string. Theoretically we should be able to use is_null() or array_key_exists() to distinguish between not set (and thus use default) and set but empty.

bennothommo commented 1 year ago

@LukeTowers that's correct, but after the first save, there's no way to differentiate.

LukeTowers commented 1 year ago

@bennothommo I don't understand, why wouldn't there be? After the first save those variables are set, even if they're empty and then the default value will not apply or show up.

bennothommo commented 1 year ago

The problem is that the theme developers might assume that the default value should show up if it's empty. Currently the only way to achieve allowing a default value in place of an empty string is to use a variable block and a conditional.

LukeTowers commented 1 year ago

@bennothommo I don't think that's a major concern tbh, there is already established precedence in Winter of default values not showing up for records that already exist - i.e. the form logic does not apply default values in any context other than create regardless of the emptiness of the field in question.

bennothommo commented 1 year ago

Could we perhaps include an option defaultOnEmpty that would show the default value if blank?

LukeTowers commented 1 year ago

What's the concern that warrants adding that as an option @bennothommo?

bennothommo commented 1 year ago

I know a couple of times during my early days the following scenario caught me out:

{text name="submenuHeader" label="Submenu header"}Pages{/text}

// List of sub-pages underneath

I would be expecting it to use Pages nearly all the time, unless I specified a different header, like Resources. But because saving the page resulted in submenuHeader being blank unless I specified something, the submenu header simply disappeared.

I can't recall if I ever tested the following:

{text name="submenuHeader" label="Submenu header" default="Pages"}Pages{/text}

// List of sub-pages underneath

But seems to be a redundant "default" call given the content within is supposed to be the default.

Maybe it might be worth trying to ensure the default contents (the content within the tags) get populated as default values in the form? That might be cleaner.

LukeTowers commented 1 year ago

@bennothommo ah, I see. Yes, I was assuming that the contents inside of the tag was being treated as the value for the default attribute, I would be fine with focusing on supporting that as the solution here.

LukeTowers commented 3 weeks ago

If anyone cares about this we can open another PR, issue, or discussion to implement the proposed solution of supporting loading the "default" value from the tag as the default for the field.