-
Notifications
You must be signed in to change notification settings - Fork 11.7k
fix: show bookings-v3 only in the settings but not in the banner #27330
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
fix: show bookings-v3 only in the settings but not in the banner #27330
Conversation
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Update HAS_*_OPT_IN_FEATURES constants to filter by 'settings' location - Add getOptInFeaturesForScopeInSettings() helper function - Update FeatureOptInService.listFeaturesForUser() to filter by 'settings' - Update FeatureOptInService.listFeaturesForTeam() to filter by 'settings' - Update useFeatureOptInBanner hook to check for 'banner' location Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
…etOptInFeaturesForScope Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
| height: 348, | ||
| }, | ||
| policy: "permissive", | ||
| displayLocations: ["settings"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all we will need to do make the banner appear on /bookings is ↓
displayLocations: ["banner", "settings"],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 5 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 5 files
sean-brydon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - nice and clean
Merge activity
|
What does this PR do?
Adds a
displayLocationsproperty to theOptInFeatureConfiginterface to control where opt-in features are displayed (settings page, banner, or both), and applies this filtering at the callers' side.How to test locally
Run it to globally enable the flag, and then go to the settings page to see the "Features" menu.
Changes:
OptInFeatureDisplayLocationtype with values"settings"|"banner"displayLocationsproperty toOptInFeatureConfiginterfacegetFeatureDisplayLocations()- returns display locations with default of['settings']shouldDisplayFeatureAt()- checks if a feature should display at a specific locationgetOptInFeaturesForLocation()- filters features by locationgetOptInFeaturesForScope()to accept an optionaldisplayLocationparameter for filteringHAS_*_OPT_IN_FEATURESconstants to usegetOptInFeaturesForScope(scope, "settings").length > 0FeatureOptInService.listFeaturesForUser()to filter by 'settings' locationFeatureOptInService.listFeaturesForTeam()to filter by 'settings' locationuseFeatureOptInBannerhook to check for 'banner' location before showingDefault behavior: If
displayLocationsis omitted, features default to['settings']only.Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Verify the helper functions work as expected:
getFeatureDisplayLocations({ slug: "test", ... })should return["settings"](default)getFeatureDisplayLocations({ slug: "test", displayLocations: ["banner"] })should return["banner"]shouldDisplayFeatureAt(feature, "settings")should returntruefor features withoutdisplayLocationsgetOptInFeaturesForScope("user", "banner")should only return user-scoped features with"banner"in theirdisplayLocationsVerify caller-side filtering:
listFeaturesForUser()andlistFeaturesForTeam()should only return features with 'settings' in displayLocationsHAS_*_OPT_IN_FEATURESconstants should only be true if there are features with 'settings' locationChecklist
Human Review Checklist
['settings']) matches requirementsdisplayLocationparameter - the filtering logic ingetOptInFeaturesForScopeisn't directly tested. Consider if this is acceptable or if tests should be added.Link to Devin run: https://app.devin.ai/sessions/a064ee43a56d458caf2892b55959f1ea
Requested by: @eunjae-lee