search

AmpersandJS / ampersand-select-view

Select field for ampersand-form-views
MIT License
11 stars 26 forks source link

Please don't set the label to opts.name #44

Closed mrDarcyMurphy closed 9 years ago

mrDarcyMurphy commented 9 years ago

Line 38:

this.label = opts.label || this.name; // init as string. render() mutates into DOM el with .textContent equal to this value

The way I'm using the select-view, when I deliberately set the label to nothing that means I don't want a label to appear. Additionally, the name I pass into the select-view options usually matches the model attribute and is rarely (if ever) a human friendly string.

If it has to be a string, then at least set it to one. I know an empty string returns false, but I didn't see any further check, and this way it would at least avoid unintended side effects of setting it to the name.

this.label = opts.label || "";
cdaringe commented 9 years ago

hey @mrDarcyMurphy. the module is geared to show the label based on presence of the label el, regardless of the label content. same thing goes for the validation messages & message containers. in your app, rather than setting the label to the empty string, can you instead just hide the label? both still one line calls--selectView.el.querySelector('[data-hook~=label]').style.display = 'none'

wraithgar commented 9 years ago

A more strict check for opts.label being undefined could mitigate this. set opts.label to '' and it won't override.

cdaringe commented 9 years ago

Ah, I see. When it was "stated doesn't show up", I was literally thinking the node itself. We should def add the undef check

wraithgar commented 9 years ago

Would really like to see this in 4.0.0 so waiting a bit before releasing.

cdaringe commented 9 years ago

i'll push somethin momentarily to review

mrDarcyMurphy commented 9 years ago

Thanks, guys. Sorry for the confusion, @cdaringe.

I still think that setting the label to the name is counter-intuitive though. /twocents

wraithgar commented 9 years ago

This is the right time to bring it up. If there's no label should we just leave the label DOM alone?

cdaringe commented 9 years ago

@mrDarcyMurphy, nah, you're cool, it was me reading hastily that led to my own befuddled-ness

@all, it is possible that styles against the text-less node get rendered, which was my original comment suggested to hide the node. i can see people both wanting the node present consistently for a simple contract of when the node is present, and people not wanting it for possible aesthetics.

it's more overhead, but maybe we toggle it's display to 'none' when label is falsy, that way all potential needs can be satisfied?

mrDarcyMurphy commented 9 years ago

@cdaringe Sorry for the late reply, been busy shipping.

I think that toggling the label's display is better than putting the name in. It's also consistent with ampersand-input-view that way.