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.

Sign up for GitHub

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: Announcement feature flag per page #4218

Merged
merged 7 commits into from
Jun 28, 2024
Merged

feat: Announcement feature flag per page #4218

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
69369ab
feat: Announcement feature flag per page
novakzaballa Jun 24, 2024
05e109b
Add AnnouncementPerPage component
novakzaballa Jun 25, 2024
3b4b45a
Add Announcement component
novakzaballa Jun 25, 2024
c461bf0
Merge branch 'main' into feat/announcement-feature-flag-per-page
novakzaballa Jun 25, 2024
94d4646
Update how the announcements are displayed
novakzaballa Jun 26, 2024
971c2fc
Solve no FF value issue
novakzaballa Jun 26, 2024
8c7a2a0
Use machPath
novakzaballa Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions frontend/web/components/AnnouncementPerPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React, { FC } from 'react'
import InfoMessage from './InfoMessage'
import flagsmith from 'flagsmith'

type AnnouncementPerPageValueType = {
id: string
title: string
description: string
isClosable: boolean
buttonText: string
url: string
}

type AnnouncementPerPageType = {
announcementPerPageValue: AnnouncementPerPageValueType
}

const AnnouncementPerPage: FC<AnnouncementPerPageType> = ({
announcementPerPageValue,
}) => {
const { buttonText, description, id, isClosable, title, url } =
announcementPerPageValue
const closeAnnouncement = (id: string) => {
flagsmith.setTrait(`dismissed_announcement_per_page`, id)
}

return (
<div className='container mt-4'>
<div className='row'>
<InfoMessage
title={title}
isClosable={isClosable}
close={() => closeAnnouncement(id)}
buttonText={buttonText}
url={url}
>
<div>
<div>{description}</div>
</div>
</InfoMessage>
</div>
</div>
)
}

export default AnnouncementPerPage
22 changes: 22 additions & 0 deletions frontend/web/components/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import AuditLogIcon from './svg/AuditLogIcon'
import Permission from 'common/providers/Permission'
import HomeAside from './pages/HomeAside'
import ScrollToTop from './ScrollToTop'
import AnnouncementPerPage from './AnnouncementPerPage'

const App = class extends Component {
static propTypes = {
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

No need to pollute app with this logic, can go in the component

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

'dismissed_announcement_per_page',
)
const announcementPerPageValue = Utils.getFlagsmithJSONValue(
'announcement_per_page',
null,
)
const showAnnouncementPerPage =
!announcementPerPageDismissed ||
announcementPerPageDismissed !== announcementPerPageValue.id
Utils.getFlagsmithHasFeature('announcement_per_page') &&
announcementPerPageValue?.pages?.length > 0
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) =>
Copy link
Member

@kyle-ssg kyle-ssg Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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,
    })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand this should target a specific page and not particular entities.
Maybe in the future if we need it at some point.

Copy link
Member

Choose a reason for hiding this comment

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

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'

pathname.includes(page),
)
const isOrganisationSelect = document.location.pathname === '/organisations'
const integrations = Object.keys(
JSON.parse(Utils.getFlagsmithValue('integration_data') || '{}'),
Expand Down Expand Up @@ -409,6 +425,12 @@ const App = class extends Component {
</div>
</div>
)}
{user && showAnnouncementPerPage && announcementInPage && (
<AnnouncementPerPage
Copy link
Member

Choose a reason for hiding this comment

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

Might make more sense to just call this <Announcement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned before I prefer to have 2 different components.

announcementPerPageValue={announcementPerPageValue}
/>
)}

{this.props.children}
</Fragment>
)}
Expand Down
Loading