feat: Announcement feature flag per page #4218
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
github-actions
bot
added
front-end
|
Uffizzi Preview |
|
@novakzaballa I think we discussed creating a different component for this. What made you change your mind? If we stick with the announcement feature, I think we want to consider a couple of extra things:
|
github-actions
bot
added
feature
frontend/web/components/App.js
Outdated
| @@ -350,12 +351,27 @@ const App = class extends Component { | |||
| return <div>{this.props.children}</div> | |||
| } | |||
| const announcementValue = Utils.getFlagsmithJSONValue('announcement', null) | |||
| const announcementPerPageDismissed = flagsmith.getTrait( | |||
There was a problem hiding this comment.
No need to pollute app with this logic, can go in the component
There was a problem hiding this comment.
Also on this, does a page-specific announcement take priority over the announcement banner? It makes sense to combine all of this into 1 component.
There was a problem hiding this comment.
I want to keep them separated so we can show both at the same time, a general announcement and a page-specific announcement. Of course, I'll resume as much logic as possible, however, these components are very simple, anyway.
frontend/web/components/App.js
Outdated
| @@ -409,6 +425,12 @@ const App = class extends Component { | |||
| </div> | |||
| </div> | |||
| )} | |||
| {user && showAnnouncementPerPage && announcementInPage && ( | |||
| <AnnouncementPerPage | |||
There was a problem hiding this comment.
Might make more sense to just call this <Announcement
There was a problem hiding this comment.
As mentioned before I prefer to have 2 different components.
frontend/web/components/App.js
Outdated
| const dismissed = flagsmith.getTrait('dismissed_announcement') | ||
| const showBanner = | ||
| announcementValue && | ||
| (!dismissed || dismissed !== announcementValue.id) && | ||
| Utils.getFlagsmithHasFeature('announcement') && | ||
| this.state.showAnnouncement | ||
| const announcementInPage = announcementPerPageValue?.pages?.some((page) => |
There was a problem hiding this comment.
Hmm, would this work for urls with ids or does this need to match in a more detailed way?
If it's the latter, note examples like this
matchPath(pathname, {
exact: false,
path: '/project/:projectId',
strict: false,
})
There was a problem hiding this comment.
I understand this should target a specific page and not particular entities.
Maybe in the future if we need it at some point.
There was a problem hiding this comment.
Hmm I don't understand how this will work, how would this target a page in remote config?
Nearly every page has ids in the pathname e.g.
'/project/x/environment/y/features'
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
Thanks for submitting a PR! Please check the boxes below:
pre-committo check lintingdocs/if required so people know about the feature!Changes
Now the UI can read a list of pages from the
announcement_per_pagefeature flag and display the announcement only on those pages, this is the pages name list:login
not-found
signup
home
github-setup
maintenance
password-reset
features
change-requests
scheduled-changes
change-request
scheduled-change
widget
invite
invite-link
broken
oauth
saml
environment-settings
sdk-keys
integrations
users
user-id
user
create-environment
project-settings-in-environment
compare
feature-history
feature-history-detail
project-settings
permissions
segments
organisation-settings
organisation-permissions
organisation-usage
organisation-settings-redirect
organisation-projects
account-settings
audit-log
organisations
audit-log-item
create-organisation
project-redirect
account
How did you test this code?
announcement_per_pageFF with a new key named "pages", and add it the page names for example: ["segments", "users"]