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

Implemented Muting/Unmuting of streams and topics #415

Merged
merged 4 commits into from May 23, 2017

Conversation

Projects
None yet
4 participants
@kunall17
Copy link
Contributor

kunall17 commented Mar 8, 2017

Fixes #267, opened a PR in the server zulip/zulip#3983 for implementing the routes!

@smarx

This comment has been minimized.

Copy link

smarx commented Mar 8, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch 4 times, most recently from 651ac6a to 4a37810 Mar 8, 2017

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented Apr 17, 2017

Just to update you on this. The PR is mostly good, I got few comments, but will make them later.
We are waiting on another 'long press' PR and then we'll have these PRs rebased against master.

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 4, 2017

@kunall17, your pull request has developed a merge conflict! Please review the most recent commit (2f1813a) for conflicting changes and resolve your pull request's merge conflicts.

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 5, 2017

Let's split this in few PRs?
Support for messages, reducers etc. and then the UI.

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch from 4a37810 to 3397e14 May 9, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 9, 2017

@borisyankov Updated the branch, this does not have the reducers.
PR for the server also updated.

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 10, 2017

The server PR just got merged in, the endpoints will be deployed in Zulip 1.6 :)

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch 3 times, most recently from 0f5a5f9 to 7d01481 May 11, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 11, 2017

@borisyankov Updated this one as well!

@borisyankov

This comment has been minimized.

Copy link
Contributor

borisyankov commented May 11, 2017

Great. The way you are handling the action sheet is generally correct, I just don't like the component's API and will check if the other almost similarly called component https://github.com/expo/react-native-action-sheet is a better fit (or you can check too, if you want)

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 12, 2017

I'll check it out!

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch from b2d7864 to 2856f06 May 14, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 14, 2017

@borisyankov Used expo action sheet

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch from 2856f06 to c1029f8 May 14, 2017

@zulipbot

This comment has been minimized.

Copy link
Member

zulipbot commented May 19, 2017

Heads up @kunall17, we just merged some commits (latest: 8213182) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch from c1029f8 to 9a72802 May 21, 2017

.eslintrc Outdated
@@ -72,7 +72,8 @@
"chatbubbles", "nonexisting", "primarytext", "autocomple", "stringify", "backend",
"Menlo", "ok", "py", "todo", "Mc", "lodash", "selectable", "isequal", "lightgray", "tc",
"zulipchat", "prepend", "pierre", "allen", "jan", "donald", "jane", "unicode", "joe",
"dan", "abramov", "lang", "Crashlytics", "bool", "ionicons", "truthy", "bezier", "decrement", "js", "jsonp",
"dan", "abramov", "lang", "Crashlytics", "bool", "ionicons", "truthy", "bezier", "decrement",
"jsonp", "js", "actionsheet", "ionicons", "Un", "Unmute"

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

You will not need the 'Un' here if you rename everything from 'unMute' to 'unmute' (which is more correct)

>
<NavigationContainer />
</IntlProvider>
<ActionSheetProvider>

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

Do we have to have the ActionSheetProvider here? Maybe inside the Chat.js or even MessageList?

autoScrollToBottom = false;

constructor(props) {
super();
this.state = { actionSheetButtons: ['', ''] };

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

Think if you can implement this feature without using state?
I am not sure how I'll try to help with ideas.

'Reply',
'Copy to clipboard',
'Cancel'
const unMuteTopicHelper = ({ auth, message }) => {

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

Definitely not liking 'helper' in names of functions and anything. Usually don't mean anything.


const skip = () => false;

const actionSheetButtons = [

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

We'll need to translate all these strings.
Ping me when you finish the other things to decide how to do it.

// If skip then covered in constructActionButtons
{ title: 'Unmute Topic', onPress: unMuteTopicHelper, onlyIf: skip },
{ title: 'Mute Topic', onPress: muteTopicHelper, onlyIf: skip },
{ title: 'Mute Stream', onPress: muteStreamHelper, onlyIf: skip },

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

use small cases, so not 'Mute Stream' but 'Mute stream'

// These are dependent conditions, hence better if we manage here rather than using onlyIf
if (message.type === 'stream') {
if (isTopicMuted(message, mute)) {
buttons.push('Unmute Topic');

This comment has been minimized.

@borisyankov

borisyankov May 22, 2017

Contributor

We will want to implement this a bit differently, so we don't use raw strings (that we need to translate).

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch 2 times, most recently from 8ca73e7 to b096e2b May 23, 2017

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented May 23, 2017

Updated :)

@kunall17 kunall17 force-pushed the kunall17:patch-4 branch from b096e2b to 4ef6154 May 23, 2017

@borisyankov borisyankov merged commit a5b50a9 into zulip:master May 23, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.