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

[ISSUE #3083] Adapted chat menu to latest mockups #3308

Merged
merged 1 commit into from
Feb 16, 2018
Merged

Conversation

jeluard
Copy link
Contributor

@jeluard jeluard commented Feb 13, 2018

fixes #3083

Summary:

Adapt chat menu to reflect latest mockups

Steps to test:

  • Open Status
  • Navigate chat / group chat
  • Validate menu reflect mockups

status: ready

@jeluard jeluard self-assigned this Feb 13, 2018
@status-github-bot status-github-bot bot added this to REVIEW in Pipeline for QA Feb 13, 2018
creating? [:get :accounts/creating-account?]
group-chat? [:chat :group-chat]
chat-name [:chat :name]
chat-id [:chat :chat-id]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why so many subscriptions, isn't better and more effective to just use [:get-current-chat] and destructure the necessary properties ?
We are eventually trying to get rid of the general :get/:set subs and events as they are anti-pattern (clearly stated in re-frame guide), so it would be best to not add them in new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't change the original design as this is more a UI PR.
Do you think we have all details with get-current-chat ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem with that, that's why I didn't requested changes, just mentioned it.
It would also save us 2 lines of code, being:

{:keys [group-chat name chat-id} [:get-current-chat]

Instead of those three lines.

Copy link
Contributor

@alwx alwx left a comment

Choose a reason for hiding this comment

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

👍

@jeluard
Copy link
Contributor Author

jeluard commented Feb 13, 2018

@janherich Updated

@yenda yenda changed the title ISSUE #3083] Adapted chat menu to latest mockups [ISSUE #3083] Adapted chat menu to latest mockups Feb 14, 2018
@annadanchenko annadanchenko moved this from TO TEST to IN TESTING in Pipeline for QA Feb 15, 2018
@annadanchenko annadanchenko self-assigned this Feb 15, 2018
@annadanchenko
Copy link

smaller suggestions area is shown
screenshot_20180215-132217

@alwx
Copy link
Contributor

alwx commented Feb 15, 2018

just a small reminder: this PR introduces a lot of changes for suggestions area #3259

@annadanchenko
Copy link

first check 3259 when it’s ready.
second: after 3259 is merged, Julien will rebase #3308 and we can proceed its testing.

Signed-off-by: Julien Eluard <julien.eluard@gmail.com>
@jeluard jeluard merged commit 058e6c5 into develop Feb 16, 2018
Pipeline for QA automation moved this from IN TESTING to DONE Feb 16, 2018
@rasom rasom deleted the feature/#3083 branch April 19, 2018 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Chat "more options", delete, and profile
5 participants