Bug 782184 - mention optional 'label' in simple-prefs docs #524

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@erikvold
Contributor

erikvold commented Aug 13, 2012

No description provided.

@wbamberg

View changes

packages/addon-kit/docs/simple-prefs.md
@@ -105,9 +105,14 @@ These are attributes that all settings *may* have:
</tr>
<tr>
+ <td><code>label</code></td>
+ <td>Button text for the `control` type.</td>

This comment has been minimized.

@wbamberg

wbamberg Aug 13, 2012

Contributor

You apparently can't use Markdown inside certain HTML block elements including tables, so this has to be control. It's painful, I know.

@wbamberg

wbamberg Aug 13, 2012

Contributor

You apparently can't use Markdown inside certain HTML block elements including tables, so this has to be control. It's painful, I know.

This comment has been minimized.

@erikvold

erikvold Aug 13, 2012

Contributor

hehe darn.. got it!

@erikvold

erikvold Aug 13, 2012

Contributor

hehe darn.. got it!

This comment has been minimized.

@wbamberg

wbamberg Aug 13, 2012

Contributor

Bah, I should not review pull requests before coffee. The table you're adding this to is "Optional Common Attributes", which contains "attributes that all settings may have". But this attribute is specific to the control type, so should be documented in the table under "Setting Types" alongside the control type - which it already is.

It's possible that this is terribly confusing, but I think if the table just presents all the attributes and lists which ones apply to which types, then it will be even more confusing.

@wbamberg

wbamberg Aug 13, 2012

Contributor

Bah, I should not review pull requests before coffee. The table you're adding this to is "Optional Common Attributes", which contains "attributes that all settings may have". But this attribute is specific to the control type, so should be documented in the table under "Setting Types" alongside the control type - which it already is.

It's possible that this is terribly confusing, but I think if the table just presents all the attributes and lists which ones apply to which types, then it will be even more confusing.

This comment has been minimized.

@erikvold

erikvold Aug 13, 2012

Contributor

I don't think there will be many optional settings.. when I was reading this page last night this is where I immediately looked for what is the label property, not seeing it there caused my to look harder which is why I made the bug.

@erikvold

erikvold Aug 13, 2012

Contributor

I don't think there will be many optional settings.. when I was reading this page last night this is where I immediately looked for what is the label property, not seeing it there caused my to look harder which is why I made the bug.

This comment has been minimized.

@wbamberg

wbamberg Aug 13, 2012

Contributor

I don't think there will be many optional settings

label isn't actually optional, is it? It's mandatory for the control type, I think. So "Optional Common Attritbutes" is the wrong table for it anyway.

There aren't many settings-specific attributes. If it's confusing to document setting-specific attributes along with the settings themselves, then this PR should do something like:

  • rename "Mandatory Common Attributes" to "Mandatory Attributes", change its description accordingly, add label as well as boolint's on and off attributes, and explain when they're used and what they mean
  • for consistency, rename "Optional Common Attributes" to "Optional Attributes" and change its description accordingly.

Or, you also merge "Mandatory Common Attributes" and "Optional Common Attributes", and describe for each attribute which types is applies to, and whether it's mandatory or optional.

@wbamberg

wbamberg Aug 13, 2012

Contributor

I don't think there will be many optional settings

label isn't actually optional, is it? It's mandatory for the control type, I think. So "Optional Common Attritbutes" is the wrong table for it anyway.

There aren't many settings-specific attributes. If it's confusing to document setting-specific attributes along with the settings themselves, then this PR should do something like:

  • rename "Mandatory Common Attributes" to "Mandatory Attributes", change its description accordingly, add label as well as boolint's on and off attributes, and explain when they're used and what they mean
  • for consistency, rename "Optional Common Attributes" to "Optional Attributes" and change its description accordingly.

Or, you also merge "Mandatory Common Attributes" and "Optional Common Attributes", and describe for each attribute which types is applies to, and whether it's mandatory or optional.

@erikvold erikvold closed this Nov 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment