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

Update autocomplete design and scroll it correctly #419

Merged
merged 2 commits into from
Aug 24, 2016

Conversation

aviraldg
Copy link
Contributor

Implements most of element-hq/element-web#1761

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@dbkr dbkr self-assigned this Aug 17, 2016
return React.cloneElement(super.renderCompletions(completions), {
className: 'mx_Autocomplete_Completion_container_block',
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Given the superclass function here is taking the array and putting it in a div wrapper, the inheritance here seems a bit redundant, since it's doing this wrapping for the overridden function to clone it, add a class, return that and throw the original away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree, in the context that it's a bit of unnecessary OOPism. I do
admit I wrote it this way expecting it would be required later. (If there's
a base structure and each layout needs to customise it a bit.)

On Wed 17 Aug, 2016, 7:26 PM David Baker, notifications@github.com wrote:

In src/autocomplete/CommandProvider.js
#419 (comment)
:

@@ -83,4 +83,10 @@ export default class CommandProvider extends AutocompleteProvider {

     return instance;
 }
  • renderCompletions(completions: [React.Component]): ?React.Component {
  •    return React.cloneElement(super.renderCompletions(completions), {
    
  •        className: 'mx_Autocomplete_Completion_container_block',
    
  •    });
    
  • }

Given the superclass function here is taking the array and putting it in a
div wrapper, the inheritance here seems a bit redundant, since it's doing
this wrapping for the overridden function to clone it, add a class, return
that and throw the original away.


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/419/files/e1739008089fdc97d4c21fb7039382764f2e92b2#r75127209,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOlRCseayDyFTzE96zslfUvGhmmQqsmks5qgxMagaJpZM4JmX4X
.

@dbkr
Copy link
Member

dbkr commented Aug 17, 2016

@matrixbot test this please

@aviraldg
Copy link
Contributor Author

@dbkr I've extracted the inline styling here and cleaned up the other bits. Should be good to go.

@ara4n
Copy link
Member

ara4n commented Aug 24, 2016

lgtm

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.

4 participants