Skip to content
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

feat(EMI-2187): Refresh Bids screen every 10 secs #11407

Merged
merged 4 commits into from
Jan 28, 2025

Conversation

MrSltun
Copy link
Member

@MrSltun MrSltun commented Jan 17, 2025

This PR resolves EMI-2187

Description

This PR implements a useInterval in the Bids screen to update it every 10 seconds

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • Refresh MyBids screen every 10 secs

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Jan 17, 2025

This PR contains the following changes:

  • Cross-platform user-facing changes (Refresh MyBids screen every 10 secs - MrSltun)

Generated by 🚫 dangerJS against 8f4bb1b

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

This approach is totally fine, but heads up that we use this usePoll hook throughout a few of our codebases. Might be nice to add here as well: https://github.com/artsy/force/blob/main/src/Utils/Hooks/usePoll.ts

@MrSltun
Copy link
Member Author

MrSltun commented Jan 20, 2025

@damassi I saw that usePoll is being used in Force (and probably other places) but I noticed that the implementation is very similar to the useInterval hook that we have in Eigen, and I saw that we use useInterval in a few places as well.

A good Future Friday project idea would be to consolidate them into one across our codebases :)

Copy link
Contributor

@erikdstock erikdstock left a comment

Choose a reason for hiding this comment

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

L G T M

Comment on lines 38 to 51
relay.refetch(
{},
null,
(error) => {
if (error) {
console.error("MyBids/index.tsx #refreshMyBids", error.message)
// FIXME: Handle error
}
setIsFetching(false)
},
{ force: true }
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: If we aren't going to handle it explicitly, could we send to sentry or otherwise know how often this happens?

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea! I'll add that

}

useAppState({
onChange: (state) => {
setAppIsInForeground(state === "active")
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: should "active" be constantized/enumized?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think since it's typed, it's okay to leave it as is?

@damassi
Copy link
Member

damassi commented Jan 21, 2025

Ah! Sorry @MrSltun - i read useInterval as setInterval -- same thing, np 👍

@MrSltun MrSltun requested a review from gkartalis January 27, 2025 12:36
@MrSltun MrSltun force-pushed the mrsltun/EMI-2187/refreshing-Bids-screen branch from b95554e to 8f4bb1b Compare January 27, 2025 12:41
@MrSltun MrSltun merged commit fba143a into main Jan 28, 2025
7 checks passed
@MrSltun MrSltun deleted the mrsltun/EMI-2187/refreshing-Bids-screen branch January 28, 2025 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants