Skip to content

Make LHN rows look the same as chatswitcher ones#793

Merged
robertjchen merged 8 commits into
masterfrom
alberto-styleLHNRows
Nov 19, 2020
Merged

Make LHN rows look the same as chatswitcher ones#793
robertjchen merged 8 commits into
masterfrom
alberto-styleLHNRows

Conversation

@Gonals

@Gonals Gonals commented Nov 13, 2020

Copy link
Copy Markdown
Contributor

@shawnborton, can you review (although I have reused the ChatSwitcher file, so the styles should be fine)?
@marcaaron, can you review? Any reason we might want to keep these two in separate files?

Instead of applying the styles to SideBarLink.js, I decided, instead, to use ChatSwitcherRow (now renamed to ChatLinkRow in both places, since we'll likely always want to keep the same format in both places and it just required a few changes.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/144292

Tests/QA

  1. Open Native Chat and open a few chats in the LHN (you might need to pin them). Single-participant DMs should show the icon, the name of the person (if they have one set) and the login below. Reports should show the report's name and group DMs, a list of the participants' names (basically, the same as the chat switcher):

Screen Shot 2020-11-12 at 1 11 56 PM

2. Make sure receiving a message correctly bolds the row in the LHN:

Screen Shot 2020-11-16 at 10 31 13 AM

3. Make sure ChatSwitcher still works correctly. 4. Check that 1-3 are all true across platforms

@Gonals Gonals requested a review from a team as a code owner November 13, 2020 10:04
@Gonals Gonals self-assigned this Nov 13, 2020
@botify botify requested review from robertjchen and removed request for a team November 13, 2020 10:04
@shawnborton

Copy link
Copy Markdown
Contributor

Lookin' pretty good! One small thing though - when a chat row is unread, the email address part should not be bold and white, it should stay as it is:
image

@marcaaron

Copy link
Copy Markdown
Contributor

Gonna tag in pullerbear on this one since I am leaving for a week + and won't have time to stick with this.

@marcaaron marcaaron requested review from a team and removed request for marcaaron November 13, 2020 19:10
@botify botify requested review from roryabraham and removed request for a team November 13, 2020 19:10
isChatSwitcher: false,
};

const ChatLinkRow = ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just ChatRow would be good enough.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but it felt like a row in the chat itself, instead of a row with a chat 🤷

@Gonals

Gonals commented Nov 16, 2020

Copy link
Copy Markdown
Contributor Author

Lookin' pretty good! One small thing though - when a chat row is unread, the email address part should not be bold and white, it should stay as it is:
image

Like this?
Uploading Screen Shot 2020-11-16 at 10.31.13 AM.png…

@shawnborton

Copy link
Copy Markdown
Contributor

@Gonals looks like the screenshot didn't quite come through - mind re-uploading?

@Gonals

Gonals commented Nov 18, 2020

Copy link
Copy Markdown
Contributor Author

@Gonals looks like the screenshot didn't quite come through - mind re-uploading?
Sure thing (but I updated it in the main post too!)

Screen Shot 2020-11-16 at 10 31 13 AM

@Gonals

Gonals commented Nov 18, 2020

Copy link
Copy Markdown
Contributor Author

@robertjchen, @roryabraham, bumping this!

@roryabraham roryabraham left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@roryabraham

Copy link
Copy Markdown
Contributor

You don't have to do this in this PR if you don't want to, but react-native-web v14 was just released and offers support for hover states on pressables like so:

<Pressable
  children={
    ({ pressed, hovered, focused }) => {}
  }
  style={
    ({ pressed, hovered, focused }) => {}
  }
/>

And it would be awesome if the chat switcher automatically focused the ChatLinkRow that's hovered.

@robertjchen robertjchen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great! 👍

@robertjchen robertjchen merged commit ef1281b into master Nov 19, 2020
@robertjchen robertjchen deleted the alberto-styleLHNRows branch November 19, 2020 02:43
@Gonals

Gonals commented Nov 19, 2020

Copy link
Copy Markdown
Contributor Author

You don't have to do this in this PR if you don't want to, but react-native-web v14 was just released and offers support for hover states on pressables like so:

<Pressable
  children={
    ({ pressed, hovered, focused }) => {}
  }
  style={
    ({ pressed, hovered, focused }) => {}
  }
/>

And it would be awesome if the chat switcher automatically focused the ChatLinkRow that's hovered.

This is actually really nice! There are big plans for a LHN revamp, though, so I'll probably wait till that's decided!

@tgolen tgolen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lint warnings/errors will show up on everyone's PRs, so we should avoid merging code that has them. @Gonals Can you submit a PR to have these fixed, please?

@Gonals Gonals mentioned this pull request Nov 20, 2020
to={ROUTES.getReportRoute(option.reportID)}
style={styles.textDecorationNoLine}
>
<TouchableOpacity

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI, this broke mobile clients because the touchable opacity is on top of the pressable link and this prevents the link from triggering a router change.

elirangoshen added a commit to callstack-internal/Expensify-App that referenced this pull request May 28, 2026
Brings the companion build in line with the latest commits on
Expensify/react-native-onyx#793:
- align multiGet cache hit check with cache.hasCacheForKey
- preserve cache-first merge when multiGet pre-warm rejects
- multiGet: prefer cache over stale storage on concurrent writes
- additional regression tests for mergeCollection pre-warm

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants