bgrins / ExpandingTextareas

jQuery plugin for elegant expanding textareas
http://bgrins.github.com/ExpandingTextareas/
MIT License
261 stars 73 forks source link

Getting Pre Value to Match Textarea #16

Open jg314 opened 12 years ago

jg314 commented 12 years ago

I figured it would be better to move this to its own issue. Take a look at http://jsfiddle.net/pmXxK/15/ and let me know what you think. It checks the box-sizing css property and uses width() or outerWidth() accordingly.

The other option may be to never clone the padding-left, padding-right, margin-left and margin-right and always calculate the width of the pre using outerWidth(true);

bgrins commented 12 years ago

I added in a border-box sized textarea to the demo here: http://jsfiddle.net/bgrins/pmXxK/17/.

I noticed that it doesn't work in Firefox. Totally agree that we need to get this issue taken care of, still not sure if this is the ideal solution, but at least it is a solution if we can get it working with FF.

Note: I did look into this a while ago in the cleaner-css branch (see commits here: https://github.com/bgrins/ExpandingTextareas/commits/cleaner-css). I will take a look through to try and remember what route I went down back then - I remember running into these same issues.

jg314 commented 12 years ago

It looks like there are two issues here.

1) Firefox doesn't recognize 'box-sizing'. It only recognizes '-moz-box-sizing' so we have to check for that separately.
2) jQuery doesn't render outerWidth() the same for Firefox and other browsers.

I put together a new fiddle at http://jsfiddle.net/pmXxK/20/ that shows what looks to be a working solution. I tested in in the newest versions of Chrome, Firefox and IE. It definitely needs to be cleaned up, but I guess it's a start.

Jonathan

bgrins commented 12 years ago

Good idea. Try this one out for size: http://jsfiddle.net/bgrins/pmXxK/24/

I moved the box size into a different function, getWidthBoxSize, then did something a little different. Rather than trying to map the size on change (which is susceptible to weird issues with the window resizing and content not snapping to the proper spot), I set the container to be the getWidthBoxSize of the textarea on load and make the textarea (and pre) 100% width. Any thoughts on this?

This seems to handle all the issues nicely (from brief testing). I think you are right about needing to add a special case in about percentage width, and make that an extra option (maybe a flag that can be passed in or something)?

jg314 commented 12 years ago

I'm still a little confused why we can't set the textarea and pre to the width on load instead of setting the container. Would you mind explaining that a little further? That said, it seems to work well when I test it, at least from Chrome.

While it might be nice from a programming perspective for the user to pass a variable telling us whether a percentage width is used, it would be much better if we could determine that automatically. Especially since most people will want to initialize each of the textareas all at once.

bgrins commented 12 years ago

Determining whether a percentage width is set is pretty difficult if I remember correctly (you have to read through all the stylesheets included on the page, since the browser will only ever report the computed style (number of pixels).

The idea behind setting the container width is that we can map the technique closer to the original article (100% width on the textarea). That said, I think we should just get a small, simple change out there and have it work for most people.

If we did do something to hack % width in (like tell them to set the % on both the textarea and the .expandingText in the CSS, then we set the textarea and pre to 100% in js), maybe we could have them set an attribute like data-use-percentage=true as an alternative to needing to pass an option into the initialization?

Or we could try and guess the percentage width based on the textarea's width and the parent's width and not need them to do any extra options / CSS, but that might get ugly.

jg314 commented 12 years ago

What about asking the user to set the width on .expandingText to match what they want the textarea to be no matter if they use fixed pixels or not? Then set both the textarea and the pre to 100%? This would add maybe a line or two of CSS, but should fix the problem and eliminate the need for us to determine width at all in the js.

Thoughts?

bgrins commented 12 years ago

That would definitely be a good solution in many cases. Part of the problem that this plugin can hopefully solve is to be a drop in solution with no modifications necessary - so I don't necessarily want them to have to add the extra rule all the time.

What about this: we say if you want percentage width, then specify both together in the CSS rule. For the rest of the cases, we will copy the width over. And in code, we add width to the end of the cloneCSSProperties object - which will only copy the value over if different. It shouldn't copy it over (and essentially fix it to the initial computed value if you properly specify the percentage widths on both).

bgrins commented 12 years ago

Turns out I suck at typing and pushed this to master instead of the separate branch. Does this seem to fix the issue you were experiencing?

jg314 commented 12 years ago

I completely understand the desire to keep the plugin as easy to use as possible. I'm wondering if asking them to do it only when using percentages in widths is actually more confusing because it requires them to think about how they styled it. If you have them add it every time, all they need to do is add the css rule, no thinking need. I definitely see where you're coming from though.

If you don't want to have them add it every time, I think what you proposed makes a lot of sense. I also think it's much better because it eliminates the need for us to calculate width, which was clearly more complex due to all the box-sizing issues.

bgrins commented 12 years ago

Totally agree about that, it was really tricky to measure the width exactly across all different browsers - I'm impressed that you came up with an actual working solution so quickly. I think this should be good for now, and we could always update the docs to say that they should always specify the width and have it still work on fixed values even if they don't.

jg314 commented 12 years ago

I tested the updated commit in Chrome, IE9 and Firefox and it looks like it doesn't work in IE9 or Firefox (at least in the index.html examples). For IE9 the height never increased and for Firefox the widths appeared to be off. I did notice that while Chrome did work, the textarea had a width of 400px while the textareaClone had a width of 386px. Any idea why that would be?

It may be worth reverting to the old commit while these bugs are worked out.

bgrins commented 12 years ago

Thanks, I took out the width setting (but kept in the example in the docs).

I think the width thing (at least in Chrome) might have to do with box sizing. Not sure if using .width() instead of .css('width'); might take care of the issue. Can you confirm that IE9 is working now that it was reverted?

jg314 commented 12 years ago

The static width probably won't work now in any browser because the pre is still stretching to 100%. You may want to add .exampleStaticWidth .textareaClone to width: 400px.

bgrins commented 12 years ago

Good catch, it is fixed on the site now.