-
Notifications
You must be signed in to change notification settings - Fork 44
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
Subscribe change to subscribev2 #1524
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
In
In
In
In
In
Overall, the code seems to have several issues related to missing closing curly braces and incorrect references. Make the mentioned corrections in the respective files. |
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.
@corlard3y could you also give it a functional review once @abhishek-01k is done with the changes.
@abhishek-01k a loom video with a proper explanation about the changes is much appreciated on this PR.
src/helpers/channel/notifSetting.ts
Outdated
return null; | ||
} | ||
|
||
let userSetting = ''; | ||
let numberOfSettings = 0; | ||
for (let i = 0; i < settings.length; i++) { | ||
const ele = settings[i]; | ||
const enabled = ele.enabled ? 1 : 0; | ||
if (ele.enabled) numberOfSettings++; | ||
|
||
if (Object.keys(ele).includes('value')) { | ||
// slider type | ||
if (typeof ele.value == 'number') | ||
userSetting = userSetting + SLIDER_TYPE + SETTING_DELIMITER + enabled + SETTING_DELIMITER + ele.value; | ||
else { | ||
userSetting = | ||
userSetting + | ||
RANGE_TYPE + | ||
SETTING_DELIMITER + | ||
enabled + | ||
SETTING_DELIMITER + | ||
ele.value?.lower + | ||
SETTING_DELIMITER + | ||
ele.value?.upper; | ||
} | ||
} else { | ||
// boolean type | ||
userSetting = userSetting + BOOLEAN_TYPE + SETTING_DELIMITER + enabled; | ||
} | ||
if (i != settings.length - 1) userSetting = userSetting + SETTING_SEPARATOR; | ||
} | ||
return numberOfSettings + SETTING_SEPARATOR + userSetting; |
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.
- Let's break down this function into more smaller utility functions and re-use them here.
- Replace the for loop with array.reduce( preffered) or array.forEach (less preffered).
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.
Functionality looks good to me
In the file OptinNotifSettingDropdown.tsx:
In the file InputSlider.tsx:
Overall, the missing closing braces and incorrect references should be fixed. In the file RangeSlider.tsx: No content provided. File: src/helpers/channel/notifSetting.ts:
After addressing the above issues, review the remaining logic to ensure all functionalities are correct. " All looks good. " |
|
In the file 'src/components/dropdowns/OptinNotifSettingDropdown.tsx':
In the file 'src/components/reusables/sliders/InputSlider.tsx':
In the file 'src/helpers/channel/notifSetting.ts':
Once these issues are addressed, you can re-run the code and test it. |
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.
Non functional minor fixes, but I am approving it.
Next steps:
- Get this tested from Kartikey
- Ask him to post his QA finding on the ticket.
- If Qa is successful merge the PR and ask @rohitmalhotra1420 to close this ticket.
1f947b7
In the file src/components/dropdowns/OptinNotifSettingDropdown.tsx:
Overall, the code seems to have several syntax errors and missing closing braces. In the file src/components/reusables/sliders/InputSlider.tsx:
There are several syntax errors and missing braces in this file as well. In the file src/helpers/channel/notifSetting.ts:
There are several incomplete conditional statements and missing braces in this file. Kindly revise these issues and ensure proper syntax and brace matching for the code to be error-free. All looks good. |
I have done the QA for this, the opt-in feature is working and also the notification settings are working without any issues. It seems well to me |
Changed the subscribe API to subscribeV2 for Opt In and now the user settings are getting updated as per the API call.