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.

Sign up for GitHub

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

Jump to bottom

ensure chat headers are visible #4693

Closed
wants to merge 1 commit into from
Closed

ensure chat headers are visible #4693

wants to merge 1 commit into from

Conversation

rcullito
Copy link
Contributor

@rcullito rcullito commented Jun 8, 2018 •
edited
Loading

Fix for #4249 around visibility of chat headers.

@rcullito rcullito changed the title remove icon from public chat, # is enough refactor chat headers Jun 8, 2018
@rcullito rcullito changed the title refactor chat headers WIP refactor chat headers Jun 8, 2018
rcullito
rcullito commented Jun 8, 2018
View reviewed changes
src/status_im/ui/screens/desktop/main/chat/views.cljs Outdated
chat-name]]
[react/view {:style {:flex-direction :row}}
(when public?
[react/text "Public Chat"])]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

per: https://app.zeplin.io/project/5a324ae5d60d197e7e3369cc/screen/5b16ef8daeac41933f425aa9

@rcullito rcullito changed the title WIP refactor chat headers refactor chat headers Jun 8, 2018
@rcullito rcullito requested review from vitvly, maxhora and vkjr June 8, 2018 16:51
@asemiankevich
Copy link
Contributor

@rcullito please refer to any certain zeplin mock if any is implemented in PR

@churik churik self-assigned this Jun 11, 2018
@rcullito
Copy link
Contributor Author

rcullito commented Jun 11, 2018 •
edited by ghost
Loading

@asemiankevich here is the most relevant link: https://app.zeplin.io/project/5a324ae5d60d197e7e3369cc/screen/5b16ef8daeac41933f425aa9

This PR does not completely match that design, but in particular I thought it would be convenient to add the text "public chat" while in the midst of fixing the "white panel issue" which this PR is meant to address. Let me know if any part of it is unclear! Thanks

Zeplin
Collaboration app for UI designers and frontend developers

@churik
Copy link
Member

churik commented Jun 11, 2018 •
edited
Loading

1. Chat headers are cut when you are switching between them (chat with longer name should be started first)

Steps:

  1. log in
  2. click on +
  3. add smbd to contacts
  4. click on +
  5. join any public channel, i.e. #status
  6. switch to #status
  7. switch to 1-1 chat. Pay attention to chat header.

Actual result:

can see only first 5 chars of contact name in Chat header

Expected result:

Contact name is fully displayed

Note: is reporducible when on 3 step you join to any public chat with name longer than on 5 step
Screen:
header
Video: http://take.ms/IZVNZ
OS: Mac OS High Sierra

@churik
Copy link
Member

churik commented Jun 11, 2018 •
edited
Loading

2. Cannot see differences in chat headers (new design)

Also according to new design - not sure what was implemented.
Regarding public chat header, I can see that icon was removed, all other looks completely different from design mock.

long

@rcullito
Copy link
Contributor Author

thanks for testing @churik. I will look into why the chat headers are cut as it relates to visibility issues brought up by #4249, which is what this PR aims to address.

My aim was not to address all design feedback in this PR, but to open that up in a separate PR to come after these initial usability issues are fixed. I'll change the title of this PR to address that. Sorry for the confusion!

@rcullito rcullito changed the title refactor chat headers ensure chat headers are visible Jun 12, 2018
remove icon from public chat, # is enough
bb97b4a 
explicitly add label for public chat

remove unecessary margin at top of desktop chat view

remove padding that was causing the white space which was blocking title

try to be a bit more sane with which components in the chat header are flexing
@rcullito
Copy link
Contributor Author

@asemiankevich this latest commit should ensure that titles are always visible regardless of whether one is on initial page load or coming from a public chat to an individual one.

There are still some issues of course plaguing the entire app around alignment, but hopefully this solves the visibility portion for now 🙏

Thanks for looking into it!

@rcullito rcullito assigned rcullito and unassigned churik Jun 13, 2018
@churik
Copy link
Member

churik commented Jun 18, 2018 •
edited by ghost
Loading

@rcullito
1 issue (Chat headers are cut when you are switching between them (chat with longer name should be started first) - is still reproducible
Video: http://take.ms/gTvIa, build

Also please request review from @cammellos or other mobile dev, it is required for now.
Thanks!

Monosnap screenshot tool
Monosnap — the best tool to take & share your screenshots.

This was referenced Jun 19, 2018
Closed
Closed
@churik churik mentioned this pull request Jul 2, 2018
Closed
@churik
Copy link
Member

churik commented Jul 2, 2018

@rcullito
please, take in mind that chat headers should be implemented according to design

  • 1-1 chat: zpl.io/V4J4NX4, zpl.io/am6gW4v
  • Public chat: zpl.io/2vlrMQe

@churik churik mentioned this pull request Jul 2, 2018
Closed
@rcullito
Copy link
Contributor Author

rcullito commented Jul 2, 2018 •
edited
Loading

@churik thanks for the reminder on this! I'm aware that this is open/unresolved still. Let me know if I should indicate that in any way beyond being in the column: contributor. There are some other PRs that have landed in this general area so will take another look at this today updated agains the latest desktop branch 👍

@rcullito
Copy link
Contributor Author

rcullito commented Jul 5, 2018

in recent talks it sounds like the fix for this is on the react-native side rather than achievable via styling tweaks. closing this PR for now.

@rcullito rcullito closed this Jul 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxhora maxhora maxhora approved these changes

@vkjr vkjr vkjr approved these changes

@vitvly vitvly Awaiting requested review from vitvly

Assignees

@rcullito rcullito

@asemiankevich asemiankevich

Labels
Tested - Issues
Projects
No open projects
Legacy QA Automation Project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rcullito @asemiankevich @churik @maxhora @vkjr @flexsurfer