-
Notifications
You must be signed in to change notification settings - Fork 429
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: Announcement per page FF accept params #4275
feat: Announcement per page FF accept params #4275
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Uffizzi Preview |
exact: false, | ||
path: routes[page], | ||
strict: false, | ||
}) | ||
if (match) { | ||
if ( | ||
parseInt(get(match, 'params.projectId')!) !== |
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 think this could be more generic
- Put projectId under a params object
- Check if announcementPerPageValue.params
- If there are params match all params
- String both sides rather than assuming the params are int
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.
Okay, I think I understood you, done.
|
||
if (match) { | ||
const annParams = | ||
convertObjectValuesToStrings(announcementPerPageValue?.params) || {} |
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.
Is any of the convert objects to strings necessary? Can't you just adjust line 67 to
return Object.keys(obj1).every((key) => {
return `${obj1[key]}` === `${obj2[key]}`
})
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, done
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
How did you test this code?
params
to the value of your feature flagAnnouncement_per_page
, and set the value to the IDs of the project, environment, or organisation, where you want the announcement to be displayed.The params key can have the following values: projectId, environmentId, and organisationId.(They are not mandatory)
This is an example of how the flag's value should appear with the params key.