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

[MM-11974] Change default return string for channel displayname #634

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

sudheerDev
Copy link
Contributor

Ticket Link

MM-11974

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit tests passed
  • Ran make flow to ensure type checking passed

Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

Lgtm, but can we add a unit test and a perhaps add a comment of what is expected?

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Sep 14, 2018

@enahum Added a comment for now. I will remove the line when i address the ticket https://mattermost.atlassian.net/browse/MM-12169.

i don't see forcing everywhere to return an empty string as a good solution as we need someone only for user typing scenario. So, planning on changing to an empty string as default and someone if needed by passing param in displayUsername function

@enahum
Copy link
Contributor

enahum commented Sep 14, 2018

Ok make sure to change it everywhere, redux/mobile/webapp as needed once you do.

Feel free to merge this when ready

@enahum enahum added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Sep 14, 2018
@sudheerDev
Copy link
Contributor Author

@enahum Merging this.

Plan is to change all instances as most of them were changed by me. That's the plan.

@sudheerDev sudheerDev merged commit 9d47dbf into mattermost:master Sep 14, 2018
@sudheerDev sudheerDev deleted the MM-11974 branch September 14, 2018 12:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants