search

urbit / talk

Urbit notifications and chat module
10 stars 7 forks source link

Calculate speech height based on text width #22

Closed knubie closed 8 years ago

knubie commented 8 years ago

Fixes urbit/talk#17

So to make a long story short, the react-infinite-any-height repo is just a small wrapper around react-infinite that uses a strategy similar to what I described in #17. That strategy doesn't really work for :talk due to the nature of scrolling bottom to top.

This PR uses canvas to calculate the length of text, which is actually pretty accurate and doesn't require any DOM manipulation (only downside is you have to break the text manually).

Tested this on a number of viewport sizes / message text, but could always use more testing.

At some point might want to move these calculations outside of render() and put them in the state to avoid expensive re-renders and having to use forceUpdate.

cgyarvin commented 8 years ago

@feels this is awesome...

On Thu, Jun 16, 2016 at 2:57 PM, Matthew Steedman notifications@github.com wrote:

Fixes urbit/talk#17 https://github.com/urbit/talk/issues/17

So to make a long story short, the react-infinite-any-height repo is just a small wrapper around react-infinite that uses a strategy similar to what I described in #17 https://github.com/urbit/talk/issues/17. That strategy doesn't really work for :talk due to the nature of scrolling bottom to top.

This PR uses canvas to calculate the length of text, which is actually pretty accurate and doesn't require any DOM manipulation (only downside is you have to break the text manually).

Tested this on a number of viewport sizes / message text, but could always use more testing.

At some point might want to move these calculations outside of render() and put them in the state to avoid expensive re-renders and having to use

forceUpdate.

You can view, comment on, or merge this pull request online at:

https://github.com/urbit/talk/pull/22 Commit Summary

  • Calculate speech height based on text width
  • Force update on resize

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/urbit/talk/pull/22, or mute the thread https://github.com/notifications/unsubscribe/AALyAScvmWS4AvReNYV4jNNLTVYFkpxLks5qMcbHgaJpZM4I32ff .

knubie commented 8 years ago

Hmm, something not quite right here, getting different results in than console than deployed code.. will investigate later.

knubie commented 8 years ago

Ok, found the culprit. Should be good to merge now.

galenwp commented 8 years ago

This does look awesome — sorry to be slow to reply. GitHub tells me there's a conflict, plus I'd like to test this out locally. Will look tomorrow.

knubie commented 8 years ago

No problem. I believe the conflict is just simply the MESSAGE_HEIGHT_FIRST_MARGIN_TOP changing from 36 to 16.

galenwp commented 8 years ago

Finally testing this. Looks awesome. Thanks so much for taking this on.

galenwp commented 8 years ago

Mind putting this into an arvo PR?

knubie commented 8 years ago

No problem.

galenwp commented 8 years ago

Thanks!