-
Notifications
You must be signed in to change notification settings - Fork 3
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: feature flag 'feeds' page #492
Conversation
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-492-k5otr9pu.web.app |
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 🚀
); | ||
expect(feedsNavigation).toBeUndefined(); | ||
}); | ||
}); |
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.
First UI unit tests 💯
export enum NavigationItemVariant { | ||
Text = 'text', | ||
Outlined = 'outlined', | ||
Contained = 'contained', | ||
} |
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.
[suggestion] Since we only use Text
-- we can get rid of this enum completely and the variant
variable is NavigationItem
instances.
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.
Would you be able to clarify? Do you mean removing the variant
variable all together? To change in Header.tsx variant={item.variant}
to variant={'text'}
? Although right now we only use Text
keep the flexibility in the future would be good?
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.
Yes that's my suggestion following the YAGNI
principle
Always implement things when you actually need them, never when you just foresee that you need them.
This is just a suggestion. Feel free to disregard if you're in disagreement 🙂
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.
I'm convinced
closes #477
Summary:
Adds feature flag to 'feeds' page to hide it in PROD until it's ready
Expected behavior:
If the feature flag is disabled, 'feeds' should not appear in the nav bar, nor should it exist as a route.
Testing tips:
Go into firebase and modify the feature flag values
Go into
web-app/src/app/constants/Navigation.ts
and manually modify the feature flag valuesPlease make sure these boxes are checked before submitting your pull request - thanks!
./scripts/api-tests.sh
to make sure you didn't break anything