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

Disable the message composer if we don't have permission to post to the room #247

Closed
wants to merge 3 commits into from

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Mar 23, 2016

dbkr added a commit to element-hq/element-web that referenced this pull request Mar 23, 2016
@richvdh
Copy link
Member

richvdh commented Mar 23, 2016

Looks ok, but it would make me much happier if you factored out a separate component (maybe the mx_MessageComposer_input div?) so you didn't have to null-check refs.textarea everywhere.

@richvdh richvdh assigned dbkr and unassigned richvdh Mar 23, 2016
@dbkr
Copy link
Member Author

dbkr commented Mar 23, 2016

Yeah, this would make a lot of sense because all the command history stuff could be factored out too. I wouldn't want to mix that in with this PR though.

@richvdh
Copy link
Member

richvdh commented Mar 23, 2016

Sure. So first factor it out, then make the change?

I think this looks fragile at the moment - it's very hard to be sure that you've got enough null guards in there, and it's going to be very easy to break it in future.

If you want to go ahead and merge it because time is short, I won't throw my toys out of the pram; but the only time the refactor is going to happen is when you need to do some work on the code in question. (In other words: there is never a good time to do refactoring.)

@richvdh
Copy link
Member

richvdh commented Mar 24, 2016

I'm going to take this off dave's hands and do the refactor first.

@richvdh richvdh closed this Mar 24, 2016
richvdh added a commit to element-hq/element-web that referenced this pull request Mar 24, 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.

2 participants