Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

feat: code cleanup & emoji replacement in composer #335

Merged
merged 3 commits into from
Aug 3, 2016

Conversation

aviraldg
Copy link
Contributor

@aviraldg aviraldg commented Jul 8, 2016

No description provided.

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@dbkr
Copy link
Member

dbkr commented Jul 8, 2016

@matrixbot test this please

selection,
emojiText,
null,
entityKey,
Copy link
Member

@dbkr dbkr Jul 8, 2016

Choose a reason for hiding this comment

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

Confusingly, trailing commas are now valid ES6 in object literals, array literals and imports, but not function calls...

Edit: actually babel does let you, but you do have to turn it on, and I guess we haven't

Edit 2: OK, I've added the flag to babel for trailing function commas. You may want to merge develop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:| For consistency:
http://babeljs.io/docs/plugins/syntax-trailing-function-commas/

On Fri, Jul 8, 2016 at 7:48 PM David Baker notifications@github.com wrote:

In src/RichText.js
#335 (comment)
:

  •            if (entity && entity.get('type') === 'emoji') {
    
  •                return;
    
  •            }
    
  •        }
    
  •        const selection = SelectionState.createEmpty(block.getKey())
    
  •            .set('anchorOffset', start)
    
  •            .set('focusOffset', end);
    
  •        const emojiText = plainText.substring(start, end);
    
  •        const entityKey = Entity.create('emoji', 'IMMUTABLE', { emojiUnicode: emojiText });
    
  •        newContentState = Modifier.replaceText(
    
  •            newContentState,
    
  •            selection,
    
  •            emojiText,
    
  •            null,
    
  •            entityKey,
    

Confusingly, trailing commas are now valid ES6 in object literals, array
literals and imports, but not function calls...


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/matrix-org/matrix-react-sdk/pull/335/files/b33452216875ba3ffd09747158bd5e1a12512e96#r70079753,
or mute the thread
https://github.com/notifications/unsubscribe/AAOlRLQEKE_mgIb9QfvZaMrjpUK4ZyUFks5qTlqsgaJpZM4JHyQd
.

@dbkr
Copy link
Member

dbkr commented Jul 8, 2016

Looks fine, although my cursor disappears after inserting an emoji into the composer. It works if I just carry on typing but it's a bit disconcerting.

@aviraldg
Copy link
Contributor Author

aviraldg commented Jul 8, 2016

Yeah, unfortunately :( Facebook's own impl uses the same workaround

What do you want me to do here? Strip trailing commas in functions or wait
for spec + babel transform to land?

On Fri, Jul 8, 2016 at 8:10 PM David Baker notifications@github.com wrote:

Looks fine, although my cursor disappears after inserting an emoji into
the composer. It works if I just carry on typing but it's a bit
disconcerting.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAOlRNJjk0kS7keeYedH0A18MQls8QA_ks5qTmFQgaJpZM4JHyQd
.

return str;
}

// Unused for now, due to https://github.com/facebook/draft-js/issues/414
Copy link
Member

Choose a reason for hiding this comment

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

This comment is now stale

This seems to be the best option for displaying emoji in the composer. While it means selected emoji don't actually have the selection colour applied, it's the most functional of all the options. Facebook uses the same approach.
@dbkr dbkr merged commit 1b39f02 into matrix-org:develop Aug 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants