search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.57k stars 2.91k forks source link

[Company cards] Handle company cars page reloading when changing feeds #52909

Open mountiny opened 15 hours ago

mountiny commented 15 hours ago

When changing the feed in the list or when closing the feed settings, the company card list is showing spinner incorrectly https://expensify.slack.com/archives/C06ML6X0W9L/p1732197668569719?thread_ts=1732108983.281759&cid=C06ML6X0W9L

melvin-bot[bot] commented 15 hours ago

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

joekaufmanexpensify commented 15 hours ago

@mountiny we already have https://github.com/Expensify/App/issues/52818#issuecomment-2488546640 , which is the same, no?

mountiny commented 15 hours ago

😢 yes, completely forgot, I will update this one

waterim commented 15 hours ago

Hey, Im Artem from Callstack and would like to help with this one

waterim commented 9 hours ago

@mountiny @joekaufmanexpensify The reason why we have a loader in the company cards and we dont have in expensify cards is here:

  1. This is ExpensifyCards: const isLoading = !isOffline && (!cardSettings || (cardSettings.isLoading && !cardsList)); And here (cardSettings.isLoading && !cardsList) this part will always return false, even when cardSettings.isLoading is true, because when cardsList is empty, it will return empty Object: "{}", thats why true && false returns false all the time(imo doesnt make any sense), it means that this full expression will be always false when property cardSettings exists.
  2. In CompanyCards we had the same behaviour previouslyconst isLoading = !cardFeeds || !!(cardFeeds.isLoading && !companyCards); But after this discussion we decided that it doesn't make sense to do this check for company cards as it will always return false as in the expensifyCardsPage (and it makes sense), but in this situation it makes sense also to remove this check in ExpensifyCards too, but in this situation it will add loader to expensifyCards too or just to cut off those expressions and isLoading will look like this: CompanyCards: const isLoading = !isOffline && !cardFeeds ExpensifyCards: const isLoading = !isOffline && !cardSettings

In this situation nothing will change for ExpensifyCards, but we will remove loader for CompanyCards. As we have useFocusEffect(fetchCompanyCards); and useFocusEffect(fetchExpensifyCards); we will always have isLoading: true for a component render as we are fetching data each time we are going to those pages

joekaufmanexpensify commented 9 hours ago

Got it. Does that mean we will change this for company cards further, or leave as is?

mountiny commented 8 hours ago

Lets try this and see how it feels const isLoading = !isOffline && !cardFeeds