-
-
Notifications
You must be signed in to change notification settings - Fork 740
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: add message banner API hooks #5078
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
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, I feel some changes are also in another PR you just missed rebasing master, but that's fine
import { IInternalMessageBanner } from 'interfaces/messageBanner'; | ||
import useAPI from '../useApi/useApi'; | ||
|
||
const ENDPOINT = 'api/admin/message-banners'; |
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.
Just wondering, should we just call them "banners" instead of "message-banners"? I feel we might regret in the future for not giving it a more general name
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 see "banner" as the type of component itself. Right now we have:
- DraftBanner (CR)
- MaintenanceBanner
- DemoBanner
So I think MessageBanner
(or in this case, message-banners
) is a good representation of what it does. Do you think it's worth making this rename now?
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.
Well, you know more than I do, so I trust your judgment. It was just an observation because message-banner seems weird... a banner is by definition a message you want to deliver... the fact that it's a "message banner" type is an implementation detail IMO. But again, take it for what it is: opinions without a lot of context :D
import { useUiFlag } from 'hooks/useUiFlag'; | ||
import { IInternalMessageBanner } from 'interfaces/messageBanner'; | ||
|
||
const ENDPOINT = 'api/admin/message-banners'; |
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.
This is re-defined here, I believe you can just export this const in the API definition
4d4fa70
to
9eb61c0
Compare
https://linear.app/unleash/issue/2-1510/create-message-banner-hooks-that-connect-to-the-new-api-endpoints
Adds new message banner API hooks that will allow us to do CRUD operations for message banners in the UI.