-
-
Notifications
You must be signed in to change notification settings - Fork 184
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(organizations): remove mmos feature flag TASK-1397 #5392
feat(organizations): remove mmos feature flag TASK-1397 #5392
Conversation
@@ -95,25 +93,12 @@ export const useOrganizationQuery = (params?: OrganizationQueryParams) => { | |||
|
|||
// Using a separated function to fetch the organization data to prevent | |||
// feature flag dependencies from being added to the hook | |||
const fetchOrganization = async (): Promise<Organization> => { | |||
const fetchOrganization = async (): Promise<Organization> => | |||
// `organizationUrl` is a full url with protocol and domain name, so we're |
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 a style issue: I tend to think we should move all the doc text for fetchOrganization above it. It's essentially a single line of code broken for screen length purposes. Seeing that solo await without brackets around it feels weird.
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.
So, combining both comments... Revisiting the existing comment above fetchOrganization
, seems if we're removing the feature flag, we can simplify it even more by putting fetchGet<...>
into queryFn below
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.
Thank you...nice catch. I visually ignored those comments and should've just removed everything at once. Just did it now. Also some changes to the query were required due to dependencies, since now we're using the organizationUrl
inside of the query.
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.
left one minor comment on style, lgtm otherwise
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global📣 Summary
Now the Organizations feature implemented up to this point is no longer hidden behind a feature flag
📖 Description
isMMosEnabled
feature flag was removedis_mmo
👀 Preview steps
Feature/no-change template:
?ff_isMMosEnabled=false
or cleaning your session storage