-
-
Notifications
You must be signed in to change notification settings - Fork 171
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
Rework create/edit group/chat #957
Conversation
@@ -16,11 +17,11 @@ ipcRenderer.on('DD_EVENT_BLOCKED_CONTACTS_UPDATED', (evt, payload) => { | |||
contactsStore.setState({ ...state, blockedContacts }) | |||
}) | |||
|
|||
ipcRenderer.on('DD_EVENT_CONTACTS_UPDATED', (evt, payload) => { | |||
ipcRenderer.on('DD_EVENT_CONTACTS_UPDATED', debounce((evt, payload) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Todo: I should remove this again.
/> | ||
</ArchivedChats> | ||
) | ||
} else { | ||
return ( | ||
<ChatListItem | ||
key={chatListItem.id} | ||
chatListItem={chatListItem} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you remove key attribute here? It might be an performance issue when reordering the chat list or when inserting new items at the beginning if react has no key to "identify" the items. https://reactjs.org/docs/reconciliation.html#recursing-on-children
I know we have an issue with duplicate keys sometimes but that should not be solved by removing the key. Consider that the chat list might have hundreds of items...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That wasn't on purpose. I actually added the correct ones to the map values (ContactRequestItemWrapper & ArchivedChats) which fixes the multiple error problem (we had the index of the element in the array + the chatId which in some cases where the same). So this problem is fixed with the last commit.
26e3bff
to
6d5f490
Compare
- Made create chat by contact into a wip dialog - moved ContactList into helpers, added useContacts hook
- Enforce fallback avatars to be uppercase - fix opening settings - Move out DeltaDialog in own helper component
- Implement add contact pseudo element
- Fix some styling - Implement getContacts2 dc method which allows us to not use a store
DD_EVENT_CHAT_MODIFIED to only swap out the one chat that has been modified. Also refactors ChatListItem
…-errors Rework create chat avoid react errors
DC_FUNCTION_CALL => EVENT_DC_DISPATCH EVENT_DC_CALL_METHOD_IGNORE_RETURN => EVENT_DC_DISPATCH EVENT_DC_CALL_METHOD => EVENT_DC_DISPATCH_CB DD_DC_CALL_METHOD_RETURN_<fn> => EVENT_DD_DISPATCH_RETURN_<fn>
@Jikstra I would prefer to have 2 columns for editing groups, was it an explicit UI decisison to have only one column in CreateChat view? |
Puuh. Sorry but I don't like it. Before implementing this i would prefer only adding them over the "Add member" button. As usual, i'm very opinionated and picky about ui/ux decisions, so this maybe comes around a bit mean. Ahh i'm sorry i'm very opinionated on those things. I think i'm a lot less opinionated on coding style and stuff than on this... |
Well I didn't mean to change it in the CreateChat view but only in the edit group view. So no switching of width etc. |
Okay some points why I think it's not a good idea:
In case we can figure out a good way of the 2 column approach, i'm fine with using this approach as the general "Add member" view. But I don't like doing it at one place different as at another place. But can you explain to me what makes the 2 column approach easier to use over the "one splitted list" approach? |
Ok - although I still don't get why we should reduce the usable space to 25% of the available width and add extra clicks and forth and back switches and check boxes to the add/remove user process. |
Hmm good point! The most forth/back that we currently have is because we can't create a new (verified) group from the main menu. If we implement this, zack, one click less for creating a (verified) group. Adding/removing a member is currently possible with some keystrokes and one click which is quite less. Showing the qr code is also one click. Proof me wrong, but for me it doesn't feel like Creating a Chat/Group is a Point and Click Adventure (yet) :D
We don't have more to show. We have 4 things: Group image, group name, qr code and group members. We could show the qr code, but the qr code changes whenever we change the groupname. Updating the qr code whenever we change the groupname makes the ui busy and distracts the user. Splitting the create group flow into two parts, first where we enter the groupname and the second where we add groupmembers/show the qr code would solve this, but comes with the cost that we need to throw away that group to change the name and we have one click more again. Yes we can show the member lists in two columns, but we can't use the other column in the upper part for anything else. Letting the groupname take the whole width looks weird and besides that we have nothing, as the group image is tightly coupled with the groupname.
👍 Another point which is important in my opinion is that we don't have a ui/ux guideline for the desktop client as there is for android (material) ios (that apple thing). Both platforms have a strict styling guide and many examples how to do things. We don't have that. It's not like there's one way on how to do it right with electron. Coming up with an own is not easy, needs a lot of thinking through and a good designer to have a consistent style guide. That's the reason why im copying so many things from android and the material design, because i think it works well on desktop too and because I think people are used to the android/smartphone flow. If we completely change how we interact with the desktop ui, we also need to come up with new components, buttons, maybe even colors. I don't know that shit, so i go the more safe path and try to stay as close with the android ui as possible, modifying things where it makes sense and using the telegram desktop ui as a good inspiration for a bridge between a desktop app and a mobile app. The downside is that it feels to much like an android app or whatever, but that's my bigger idea of how the desktop ui should look like. Changing this is possible, but than we need to change a lot. For ui consistence is key. |
Renaming proposals
Fix creating a group resulting in empty dialog
Fix some edit group bugs
Refactor editgroup
Displaying qr code works as expected, you need to use the latest deltachat-node (from git). Will do a new release (hopefully today).
This problem is probably an old bug? Would be fine to do this in another pr. |
in which situation does which function precisely erturn null?
…On Tue, Sep 10, 2019 at 04:38 -0700, Jikstra wrote:
The reason why we currently cant display the qr code is because rust master returns null for the qr code.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#957 (comment)
|
Missing:
ToDo (probably not in this pr):