scroll to bottom element added in channels and threads, new messages … #6672
Conversation
…button also added
|
Hello @tanmaythole, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
/check-cla |
There was a problem hiding this comment.
@tanmaythole thanks for your contribution, I've added a few comments that should be addressed, my main concern is about performance.
Another thing I did not comment in the code is that there seems to be an animated transition in the design between both states of the component that is not present in the code
| @@ -17,6 +17,8 @@ import {useServerUrl} from '@context/server'; | |||
| import {useTheme} from '@context/theme'; | |||
| import {getDateForDateLine, isCombinedUserActivityPost, isDateLine, isStartOfNewMessages, isThreadOverview, preparePostList, START_OF_NEW_MESSAGES} from '@utils/post_list'; | |||
|
|
|||
| import CompassIcon from '../compass_icon'; | |||
There was a problem hiding this comment.
Let's make this follow the same pattern as the other imports
| import CompassIcon from '../compass_icon'; | |
| import CompassIcon from '@components/compass_icon'; |
And then sort it accordingly
| const [showScrollToEndBtn, setShowScrollToEndBtn] = useState(false); | ||
| const [isNewMessages, setIsNewMessages] = useState(false); |
There was a problem hiding this comment.
In my opinion there is no need to have to re-render one of the most heavy components in the app just for this, but in order to be able to display the component either with the down chevron or the new messages text, you can extract it to it's own component instead and use references to update it
| const onScrollToEnd = () => { | ||
| scrollToIndex(0); | ||
| }; | ||
|
|
||
| const ScrollToEndView = () => ( | ||
| <Pressable | ||
| style={[ | ||
| isNewMessages ? styles.scrollToEndBadge : styles.scrollToEndBtn, | ||
| { | ||
| backgroundColor: isNewMessages ? theme.onlineIndicator : theme.sidebarBg, | ||
| }, | ||
| ]} | ||
| onPress={onScrollToEnd} | ||
| > | ||
| <CompassIcon | ||
| size={16} | ||
| name='arrow-down' | ||
| color={theme.sidebarHeaderTextColor} | ||
| /> | ||
| <Text>{isNewMessages && 'Jump To New Messages'}</Text> | ||
| </Pressable> | ||
| ); |
There was a problem hiding this comment.
See comment above. Also curious why the use of index instead of offset
| name='arrow-down' | ||
| color={theme.sidebarHeaderTextColor} | ||
| /> | ||
| <Text>{isNewMessages && 'Jump To New Messages'}</Text> |
There was a problem hiding this comment.
This needs to be translateable, also the check should be prior to mount the component instead of making the content optional.
| style={[ | ||
| isNewMessages ? styles.scrollToEndBadge : styles.scrollToEndBtn, | ||
| { | ||
| backgroundColor: isNewMessages ? theme.onlineIndicator : theme.sidebarBg, |
There was a problem hiding this comment.
Consider adding the background color to the already defined styles
|
@tanmaythole thanks for taking this work on! Based on the screenshots it looks like the UI doesn't follow the designs in Figma attached to the ticket. Could you have a look at this Figm file and ensure the UI follows this? |
|
Hello @enahum, thank you for reviewing the PR, I will look into it and fix the necessary changes you suggested. |
|
hi @matthewbirtch, Thanks for your suggestion. I will surely look into it and makes the necessary changes to the UI design, earlier I didn't have access to the Figma file. |
|
Thanks @tanmaythole. I wondered if that was the case so I opened up the permissions. You should be able to access now. |
|
hey @tanmaythole just to let you know that you can reach out to the team in our mobile v2 channel in the community server if you need help of any sort ;) |
|
@enahum Thanks for informing, I'll definitely reach out to the community if I get stuck anywhere. |
|
Hey @matthewbirtch & @enahum, I commit the changes you suggested and also follow the UI design according to Figma design. |
tanmaythole
requested review from
enahum
and removed request for
matthewbirtch
matthewbirtch
added
the
Build Apps for PR
|
Building app in separate branch. |
mattermod
removed
the
Build Apps for PR
|
@tanmaythole I think there are some problems here. I tested in the app generated from this PR and now when I go to a channel it seems to have reversed the order posts are displayed. The newest posts seem to be displaying at the top rather than the bottom. Before I go any further with this review, could you resolve this? |
|
@matthewbirtch something I notice in the code is that it does not animate the transition between the down arrow and the new messages indicator isn't that part of the design? |
matthewbirtch
added
the
Build Apps for PR
|
Building app in separate branch. |
mattermost-build
removed
the
Build Apps for PR
|
Just tested this on iOS and I no longer see the reverse scrolling, but I don't see the scroll-to-bottom button showing as expected. There does seem to be some kind of button shape showing below the post input at the bottom though (see attached). Definitely something still not right on iOS.
|
|
@matthewbirtch, I apologize for the issue, but I'm unable to debug what's wrong with iOS for this problem as it works as expected on Android. |
Are you not able to use the iOS simulator @tanmaythole? |
|
@tanmaythole I have done some changes to make the iOS version work better. I also fixed a few things more, like localization and some styles. If you have any question about my changes, feel free to ask! @matthewbirtch @enahum This is ready for re-review. |
Thank you @larkox for helping me into this. |
There was a problem hiding this comment.
@tanmaythole @larkox A few more UI details I've noticed and one UX behavior issue:
- There is no shadow on the button in iOS so it doesn't stand out enough from the channel content. Android seems to have the shadow, but not iOS. You can use these values:
shadowOpacity: 0.2, shadowOffset: {width: 0, height: 4}, shadowRadius: 4,
- We need to make the entire floating button tappable. Right now it seems that only the arrow icon is tappable. We should also extend the tap area to 48x48 for this floating button
- The padding on the left/right of the 'New replies' button is not quite right. Can you increase the horizontal padding on left/right by 4px? Also, this button does not have a shadow either on iOS.
- For the behavior 'New replies' button, the intent was that this is only used if new replies come in while you are scrolled up. Right now, this button is always showing if the new message line is present and I'm scrolled up beyond it. This may have been unclear in the notes for the design, but can we change this so the 'New replies' button only shows if new messages come in out of view while I'm scrolled up in the change? We should apply this for both the channel view and the thread view. Sorry if this was unclear in the design notes.
- Can we speed up the animation when the bottom slides up/down to 300ms (instead of 500 from what I'm seeing)
- Also, the 'New replies' button should say "New messages" in the channel view and "New replies" in the Thread view.
Let me know if you have any questions about the above
|
@tanmaythole Will you be tacking care of the new feedback? Some problems may have been small mishaps from my side (like the new replies vs. new messages), but most of them you should be able to address without a Mac device. If you are not able to address them, feel free to let me know, and I will take over. |
|
@larkox Sure, I will address them. |
|
@matthewbirtch I have made the changes that you suggested. |
|
Thanks @tanmaythole looks like the UI items are good now. However, I don't see this behavior change addressed. Let me know if you need clarity or if @larkox can help out with the logic at all:
|
|
@matthewbirtch I have made the changes that you suggested. Please check and let me know if any changes are required. |
@tanmaythole the new messages button should ONLY show if a new messages come in while you're scrolled up beyond the view of the new messages, but it seems to show any time I'm scrolled up beyond the new message line. I'm not sure if I'm clear on the requirement, but here are a few videos. I'm still seeing the new messages button showing any time I'm scrolled above the new message line. Screen.Recording.2023-06-30.at.5.20.58.PM.movScreen.Recording.2023-06-30.at.5.17.36.PM.mov |
|
@tanmaythole would you finish this or should we close it so that someone else can pick this up? |
|
Closing this PR in context of #7851 |
Scroll to the bottom element added in channels and threads, new messages button was also added.
Summary
It adds new functionality to the pull request channel and thread screen of the mobile app. The problem is there is no easy way to get to the bottom of the channel when you are scrolled up in the channel's history and users are forced to scroll manually, which can be frustrating if you have scrolled up far. This problem is solved by adding a new scroll-down button when the user scrolls up to some height by reading the previous messages. The same button notifies for incoming new messages as the user scrolls.
Ticket Link
Fixes mattermost/mattermost#21269
Checklist
Device Information
This PR was tested on: Android v12 and Android v10
Screenshots
Release Note