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

bugfix: refresh looker when media field changes #3735

Merged

Conversation

sashankaryal
Copy link
Contributor

What changes are proposed in this pull request?

Fix bug where looker wasn't refreshing correctly when modal media field changed.

2023-10-29 20 46 17

What areas of FiftyOne does this PR affect?

  • App: FiftyOne application changes
  • Build: Build and test infrastructure changes
  • Core: Core fiftyone Python library changes
  • Documentation: FiftyOne documentation changes
  • Other

@sashankaryal sashankaryal requested a review from a team October 29, 2023 15:21
@sashankaryal sashankaryal self-assigned this Oct 29, 2023
@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (release/v0.22.3@a506c03). Click here to learn what that means.

Additional details and impacted files
@@                Coverage Diff                 @@
##             release/v0.22.3    #3735   +/-   ##
==================================================
  Coverage                   ?   15.98%           
==================================================
  Files                      ?      596           
  Lines                      ?    73191           
  Branches                   ?      917           
==================================================
  Hits                       ?    11703           
  Misses                     ?    61488           
  Partials                   ?        0           
Flag Coverage Δ
app 15.98% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benjaminpkane
benjaminpkane previously approved these changes Oct 29, 2023
Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

LGTM 🖼️

One alternative that captures media field changes for all Looker components is to include useRecoilValue(selectedMediaField(true)) as a dependency in the createLooker memo

@sashankaryal
Copy link
Contributor Author

LGTM 🖼️

One alternative that captures media field changes for all Looker components is to include useRecoilValue(selectedMediaField(true)) as a dependency in the createLooker memo

Somehow missed that useMemo(). Thanks for pointing that out, much cleaner.

Copy link
Contributor

@benjaminpkane benjaminpkane left a comment

Choose a reason for hiding this comment

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

LGTM!

@sashankaryal sashankaryal merged commit 88fe97d into release/v0.22.3 Oct 30, 2023
15 checks passed
@sashankaryal sashankaryal deleted the bugfix/refresh-looker-media-field-change branch October 30, 2023 16:56
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.

None yet

2 participants