Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues 132 and 151 #152

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

share-with-me
Copy link
Contributor

No description provided.

@@ -133,8 +133,6 @@ if(modeEnabled('translation')) {
synchronizeTextareaHeights();
});

$(window).resize(synchronizeTextareaHeights);
Copy link
Member

@sushain97 sushain97 Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you figure out why this was added using git blame? I'm concerned about removing it without knowing why in case we regress on another front.

Copy link
Contributor Author

@share-with-me share-with-me Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, Nop! I tried to figure out the code that was causing this. I will use git blame now!

Copy link
Contributor Author

@share-with-me share-with-me Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using git blame, it shows that you have made the latest modification to that line with commit message as in this link. How do I infer from this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that commit just fixed a bunch of style issues. You'll have to look backwards in time a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure on it.

Copy link
Contributor Author

@share-with-me share-with-me Jun 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, the commit message says - Improve textarea mechanics.
The link to the blame where this line of code was added it link

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's not a helpful commit message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah. But I believe removing it should not cause an issue right? As in, it is just synchronizing heights! We could add it very easily in the event we need it. Inputs?

@sushain97
Copy link
Member

sushain97 commented Jun 23, 2017

I might also want the textarea to not decrease if I increase it manually then type something. Some max approach? I need to think about the ideal behavior of this more...

I remember when we added auto-adjusting text boxes, I was hesitant but didn't really have much time to go through it with a more critical eye... The behavior is hard to get right.

@share-with-me
Copy link
Contributor Author

I remember when we added auto-adjusting text boxes, I was hesitant but didn't really have much time to go through it with a more critical eye... The behavior is hard to get right.

I would wait on this to get the behaviour right :D

@Androbin
Copy link
Contributor

Androbin commented Mar 8, 2018

How about this:

The textarea has a text height (given) and a view height (controlled).
Let's call their difference space (could be positive/negative/zero).

Text deletion:

  • space == 0: decrease view height to match text height
  • space < 0: do nothing
  • space > 0: do nothing OR preserve space

Text addition:

  • space == 0: increase view height to match text height
  • space < 0: do nothing OR preserve space
  • space > 0: do nothing

@Androbin
Copy link
Contributor

Androbin commented Mar 8, 2018

@sushain97
Copy link
Member

Is the space text height - view height or vice-versa in your proposal, @Androbin ?

@Androbin
Copy link
Contributor

I define space = view height - text height

@sushain97
Copy link
Member

sushain97 commented Mar 30, 2018

Hmmm, what happens if I paste a block of text then? space > 0 before and then space < 0 after. Won't nothing happen?

@Androbin
Copy link
Contributor

I can think of three basic options:

  • never preserve space
  • always preserve space
  • don't increase space when space > 0 AND don't decrease space when space < 0

@Androbin
Copy link
Contributor

Androbin commented Mar 30, 2018

We could also ensure that space > 0 at any time.

@sushain97
Copy link
Member

@sushain97 We could also ensure that space > 0 at any time.

This isn't great. It shouldn't keep growing without a max-width.

@Androbin
Copy link
Contributor

Androbin commented Apr 4, 2018

It shouldn't keep growing without a max-width.

@sushain97 We are talking about height, aren't we?

@Androbin
Copy link
Contributor

Androbin commented Apr 4, 2018

But yeah, we could just use these CSS properties:

  • max-height: 123px
  • overscroll-y: auto

@sushain97
Copy link
Member

@sushain97 We are talking about height, aren't we?

Oops, yes. I meant width.

But yeah, we could just use these CSS properties:

Indeed. But this needs to be taken into account in the JS as well since max-height does not override a manual height setting, I think...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants