Closed johnmlee101 closed 4 months ago
So the RN upgrade is done which means this is no longer on hold. @burczu what do you think it would take to revive this and try to make it happen again? Perhaps you or someone else from Callstack could work on it?
Hey! It's Jakub from Callstack - expert contributor group - I'll take this issue over from Bartek.
Great! Thanks for taking it on @bionanek.
I'd love to take a look at this with fresh eyes, since I've completely forgotten what the problems were. Perhaps you could summarize what would need to be done to make the solution happen.
Hi @puneetlath!
The issue in this task was related to missing fix in rn web, but as you mentioned in previous comments the fix is merged. Last part from my side that needs to be addressed is based on refactor that was proceeded some time ago with removal of moment js and replacing it by date fns. I need to adjust date formatting within that area, and then the solution will be ready to go.
I also believe that you tagged other Jakub from Callstack in your recent comment :D
Hey! I didnt make much progress on that issue this week, will look into that in following days.
Hey! Im still working on this issue.
not overdue
not overdue
Hi! We have this issue https://github.com/Expensify/App/issues/21811 where onViewableItemsChanged
on web was giving false positive results. It should have been fixed as we're now using react-native-web
instead of forked repo. If you still see the issue, please comment in the original issue. Thanks! cc @burczu
Hi @MonilBhavsar , the issue with onViewableItemsChanged
has been fixed, so we now see right values.
Thanks for confirming. We'll close the other issue then. Feel free to reopen if you see any issues with that method
not overdue
Im still working on the issue.
Still working on the issue, I will try to open PR in next couple of days
Can you push up a PR to show your progress here?
Hey @johnmlee101 I have created draft PR
@MrRefactor how's it going?
Also, since it has been so long since we originally created this, @Expensify/design I wanted to check-in and see if we still feel good about using Slack-style date dividers as mocked up here: https://github.com/Expensify/App/issues/18101#issuecomment-1526330673
I actually did a few doodles to update the styles a little bit when this came up recently (our design system has evolved a little bit since this was first designed). Curious if @shawnborton and @dubielzyk-expensify (and others!) have any preferences, feedback, likes, dislikes, etc.
There's nothing really wrong with the original plan, so if everyone wants to roll with that I'm totally cool. Just wanted to take a stab at giving it a little refresh.
I think we should definitely update the design given how old this issue and given that things have changed.
Will the date pill actually be a button that you can interact with, or just a label? If it's just a label, we should use our badge pattern. Though I don't think we need a calendar icon, it feels a bit extraneous (though it does look nice!). If the label can be interacted with, I think it should be a button and would opt for something like you have on the right, though I might skip the horizontal lines so we don't get a weird behavior when the buttons become floating or not.
I think to start they will just be a non-clickable label, but once comment linking is out in the wild, we should be able to update them to be clickable.
Cool cool. I would love to see the second screenshot from Danny but without the icons, and see how that feels.
I'm for it!
Love that! Love how clean it is too.
Great! And then if we do that, we can also update the comment header to just be:
RoboPigeon 11:28 PM
Instead of:
RoboPigeon January 28 at 11:28 PM
Agreed? Since the date context is in the label. And that'll free up some more space.
Oh nice, I like that idea.
Yes to everything!! I love the simpler time stamp @puneetlath.
As for the mocks these are both excellent. I love the label one. I kinda find that Slacks one are too visually demanding so I prefer the subtle label approach.
Do you have a Figgy handy (tried finding it). Kinda wanna try something else than center aligning. I do think it's probably best being center, but part of me is keen to see if it can work over on the left or right.
@dubielzyk-expensify of course! Sorry, should've posted it before! https://www.figma.com/file/WtqVXdvroIWPLj3kgFI1nM/Mentions?type=design&node-id=993-5512&mode=design&t=51z3SP0R7ihI8rb0-11
Added a few variations in there and I hate them all 😂 What you have is gold.
Nice! Ok, so we're going with https://github.com/Expensify/App/issues/18101#issuecomment-1944529881
@MrRefactor do you have an ETA for the PR?
Nice! Ok, so we're going with #18101 (comment)
@MrRefactor do you have an ETA for the PR?
Hey @puneetlath, I need to implement new designs and address issue with correct positioning of the indicator above the messages. Apart of that rest of the implementation is ready. Will let you know when I will update the PR and mark it as ready for review.
How's it going @MrRefactor?
hey @puneetlath, I have applied design changes and working on resolving issue with changing the indicator value on scroll.
@MrRefactor do you have a draft PR up somewhere we can get early eyes on?
Hi @shawnborton, I have just pushed latest changes, right now I need to adjust one styling on web/desktop and PR will be ready to be reviewed.
PR link: https://github.com/Expensify/App/pull/35897
@puneetlath I have just resolved issue that I mentioned in my previous comment.
Sounds good, let us know when it's ready for a design review!
@shawnborton
Im working on the issue occurring on both web and desktop - due to the fact that the indicator which is positioned absolute outside of the FlatList, there is a small offset between the "static" indicator and the with position absolute parameter due to the fact that there is a scrollbar that is adjusting the width of the flatlist.
Also I can see that, the indicator "unread messages" is also positioned absolute with same offset as date indicator. How do we want to tackle that one on the web? Adjusting the position of the absolute date indicator will align with static indicators but it will be shifted a bit when there is a "unread messages" indicator.
Hmm from a technical standpoint, I'm not entirely sure what is best but I would say that we absolutely want these to line up so they feel aligned to the same center.
@Expensify/design how do these fit in with the New messages
pill? Do we already have that figured out? I hadn't thought of it until now.
Oh interesting, definitely didn't think of that either! Slack does it where the new message pill just replaces the date pill. I suppose we could do something similar?
Oh yeah that could work.
Oh interesting, definitely didn't think of that either! Slack does it where the new message pill just replaces the date pill. I suppose we could do something similar?
Implemented!
PR ready for review
This issue has not been updated in over 15 days. @puneetlath, @MrRefactor, @thesahindia eroding to Monthly issue.
P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!
Given focus, I think maybe it's best for us to just close this for now. We can always do it in the future if it becomes more of a priority. What do you think @Expensify/design @johnmlee101?
Hard to say honestly, it feels like this has added a lot of unanticipated complexity, but I still find this to be a bit of a polish that breaks expectations from other chat applications. How much more work do we expect this to take?
Its hard to say, as discussed with @puneetlath I removed "flowing" indicator, but after migration to typescript Im struggling with issues on ios/android native.
I'm a bit torn, and curious for other design team members' thoughts. On the one hand this is definitely a nice quality-of-life improvement. On the other hand, since we include the date of each message posted, it's not critically necessary for determining "when you are" while looking at a chat.
I'm also quite torn... I do love how much it simplifies our message timestamps, but at the same time, we need to consider how this will work for threads, particularly our new thread ancestry style.
Maybe we would need to follow Slack's lead there and not use this style within threads, and only use it for rooms/DMs?
we need to consider how this will work for threads, particularly our new thread ancestry style.
I had not even considered this inside threads 😳. Good point.
Feature Request: Have visible indicators when chats change days, similar to how Slack separates individual days with a bar Problem: When reading chat history, I had a hard time finding out where a discussion started a few days ago, and it required a bit of scanning each chat to see when that day occured. Solution: Add an indicator between each day of the week that changes between messages, giving indication when that message was sent in an easy way.
Slack
Whatsapp
Original report: https://expensify.slack.com/archives/C01GTK53T8Q/p1682528022290149
cc @puneetlath @flodnv
Upwork Automation - Do Not Edit