feat: added notification preference ui #784

muhammadadeeltajamul · 2023-05-04T06:31:13Z

Added Notification Preferences UI components.

JumpNav component will have a new option
Created new page that will all the active courses in which user is enrolled in
When a user will select a course, he/she will be redirected to preferences page of the selected course

Tests will be added after api integration

codecov · 2023-05-04T07:08:14Z

Codecov Report

Patch coverage: 1.17% and project coverage change: -2.60 ⚠️

Comparison is base (f2f761e) 38.63% compared to head (01f24b4) 36.04%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   38.63%   36.04%   -2.60%     
==========================================
  Files         110      121      +11     
  Lines        2288     2458     +170     
  Branches      624      652      +28     
==========================================
+ Hits          884      886       +2     
- Misses       1319     1483     +164     
- Partials       85       89       +4

Impacted Files	Coverage Δ
src/account-settings/AccountSettingsPage.jsx	`0.00% <ø> (ø)`
.../account-settings/AccountSettingsPage.messages.jsx	`100.00% <ø> (ø)`
src/data/reducers.js	`0.00% <ø> (ø)`
src/index.jsx	`0.00% <0.00%> (ø)`
...c/notification-preferences/NotificationCourses.jsx	`0.00% <0.00%> (ø)`
...cation-preferences/NotificationPreferenceGroup.jsx	`0.00% <0.00%> (ø)`
...fication-preferences/NotificationPreferenceRow.jsx	`0.00% <0.00%> (ø)`
...tification-preferences/NotificationPreferences.jsx	`0.00% <0.00%> (ø)`
src/notification-preferences/ToggleSwitch.jsx	`0.00% <0.00%> (ø)`
src/notification-preferences/data/actions.js	`0.00% <0.00%> (ø)`
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

sundasnoreen12 · 2023-05-08T11:32:59Z

src/notification-preferences/messages.js

+
+// eslint-disable-next-line import/prefer-default-export
+export const messages = defineMessages({
+  notificationHeading: {


add "Notifications" instead of "Notification" as defaultMessage: 'Notification',

sundasnoreen12 · 2023-05-09T05:34:43Z

src/notification-preferences/NotificationCourses.jsx

+            <Link
+              to={`/notifications/${course.id}`}
+            >
+              <div className="mb-4">
+                {course.name}
+              </div>
+            </Link>


forward arrow as per figma is missing

sundasnoreen12 · 2023-05-09T05:38:36Z

src/notification-preferences/NotificationPreferences.jsx

+    if (notificationStatus === IDLE_STATUS) {
+      dispatch(fetchCourseNotificationPreferences(courseId));
+    }
+  // eslint-disable-next-line react-hooks/exhaustive-deps


remove this lint disable next line

The dependencies it requires will cause few extra re-renders. So disabling lint for this line is better.

sundasnoreen12 · 2023-05-09T05:41:08Z

src/notification-preferences/NotificationPreferences.jsx

+    return null;
+  }
+  return (
+    <div className="ml-auto mr-auto" style={{ width: '828px' }}>


you can add css class for width 828px because same width is using in NotificationCourses.jsx also.

Added class

sundasnoreen12 · 2023-05-09T05:51:31Z

src/notification-preferences/NotificationPreferenceGroup.jsx

+            <span className="ml-auto mr-0 pr-2.5">{intl.formatMessage(messages.notificationHelpPush)}</span>
+          </span>
+        </div>
+        <div className="mt-3 pb-5.5">


padding bottom is 48px in figma currently its showing 64px

sundasnoreen12 · 2023-05-09T06:11:06Z

src/notification-preferences/data/service.js

+// import { getConfig } from '@edx/frontend-platform';
+// import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';


remove commented code if they are of no use

These are required imports that will be added when integrating API's.

sundasnoreen12 · 2023-05-09T06:15:03Z

src/notification-preferences/data/service.js

+  // const url = `${getConfig().LMS_BASE_URL}/api/notifications/${courseId}`;
+  // const { data } = await getAuthenticatedHttpClient().get(url);


remove commented code if they are of no use

This is API call for it that will bring data whose structure will be same as dummy data. It will be removed when integrating API's

sundasnoreen12 · 2023-05-09T06:16:21Z

src/notification-preferences/data/service.js

+  // const { data } = await getAuthenticatedHttpClient().get(url);
+  return [
+    { id: 'course-v1:edX+Supply+Demo_Course', name: 'Supply Chain Analytics' },
+    { id: 'course-v1:edX+Happiness+At+Work_Course', name: 'The Foundation of Happiness At Work' },


title issue replace with "The Foundations of Happiness at Work"

this is a dummy data... it will be removed when integrating api's.

sundasnoreen12 · 2023-05-09T06:17:20Z

src/notification-preferences/data/service.js

+  return [
+    { id: 'course-v1:edX+Supply+Demo_Course', name: 'Supply Chain Analytics' },
+    { id: 'course-v1:edX+Happiness+At+Work_Course', name: 'The Foundation of Happiness At Work' },
+    { id: 'course-v1:edX+Empathy+At+Work_Course', name: 'Empathy and Emotional Intelligence At Work' },


title issue kindly replace with "Empathy and Emotional Intelligence at Work"

Same. This is also dummy data...

src/notification-preferences/NotificationCourses.jsx

awais-ansari · 2023-05-09T08:22:09Z

src/account-settings/AccountSettingsPage.messages.jsx

@@ -565,6 +565,11 @@ const messages = defineMessages({
    defaultMessage: 'No value set.',
    description: 'The placeholder for an empty but uneditable field when there is no administrator',
  },
+  'notification.preferences.notifications.text': {


notification.preferences.notifications.label

awais-ansari · 2023-05-09T08:51:08Z

src/account-settings/JumpNav.jsx

+      {showNotificationMenu
+        && (
+        <>
+          <hr className="ml-0" style={{ width: '180px' }} />


You should not add specific width for the divider. It should inherit the width from a parent. Adjust the parent width according to Figma.

awais-ansari · 2023-05-09T08:52:26Z

src/data/reducers.js

@@ -1,11 +1,16 @@
+/* eslint-disable import/no-named-default */


default export the notificationPreferencesReducer from the component.

awais-ansari · 2023-05-09T08:53:01Z

src/data/reducers.js

+import {
+  default as notificationPreferencesReducer,
+} from '../notification-preferences/data/reducers';


import notificationPreferencesReducer from '../notification-preferences/data/reducers';

awais-ansari · 2023-05-09T09:05:08Z

src/index.scss

+.w-66 {
+  width: 66%;
+}
+
+.w-34 {
+  width: 34%;
+}
+
+.w-828px {
+  width: 828px;
+}


using responsive class from paragon.

awais-ansari · 2023-05-09T09:45:26Z

src/notification-preferences/NotificationPreferenceRow.jsx

+  preferenceName: PropTypes.string.isRequired,
+};
+
+export default injectIntl(NotificationPreferenceRow);


memoize this component

awais-ansari · 2023-05-09T09:45:50Z

src/notification-preferences/NotificationPreferenceRow.jsx

+  const onToggle = (checked, notificationFor) => {
+    dispatch(updatePreferenceValue(groupKey, preferenceName, notificationFor, checked));
+  };


memoize this callback

awais-ansari · 2023-05-09T09:47:09Z

src/notification-preferences/NotificationPreferenceRow.jsx

+  };
+  return (
+    <div className="d-flex flex-row mb-3">
+      <span className="w-66">


use col class from paragon

awais-ansari · 2023-05-09T09:47:49Z

src/notification-preferences/ToggleSwitch.jsx

+  onChange: () => null,
+};
+
+export default ToggleSwitch;


memoize this component

awais-ansari · 2023-05-09T09:54:13Z

src/notification-preferences/data/selectors.js

+export const getCourse = courseId => state => (
+  state.notificationPreferences.courses.courses.find(
+    element => element.id === courseId,
+  )
+);


change element to course.
call getCourseList selector instead of state.notificationPreferences.courses.courses

sundasnoreen12 · 2023-05-13T20:00:12Z

src/notification-preferences/NotificationCourses.jsx

+  if (courseStatus === LOADING_STATUS) {
+    return (
+      <div className="d-flex h-100">
+        <Spinner
+          variant="primary"
+          animation="border"
+          className="mx-auto my-auto"
+          style={{ width: '4rem', height: '4rem' }}
+        />
+      </div>
+    );
+  }


you can memo this like

const SpinnerComp = useMemo(() => {
if (courseStatus === LOADING_STATUS) {
return (

<Spinner
variant="primary"
animation="border"
className="mx-auto my-auto"
style={{ width: '4rem', height: '4rem' }}
/>

);
}
return null;
}, [courseStatus]);
if (SpinnerComp) { return SpinnerComp; }

Since this component is not being rendered multiple times, hence memoization is not required. Excessive memoization should be avoided in code

sundasnoreen12 · 2023-05-13T20:01:56Z

src/notification-preferences/NotificationCourses.jsx

+  intl: intlShape.isRequired,
+};
+
+export default injectIntl(NotificationCourses);


memo this
export default injectIntl(React.memo(NotificationCourses));

NotificationCourses component is not taking any props (except intl). I don't think that React.memo will be helpful

src/notification-preferences/NotificationPreferenceGroup.jsx

sundasnoreen12 · 2023-05-13T20:06:23Z

src/notification-preferences/NotificationPreferenceRow.jsx

+  const preference = useSelector(getPreferenceAttribute(groupId, preferenceName));
+  const onToggle = useCallback((checked, notificationChannel) => {
+    dispatch(updatePreferenceValue(groupId, preferenceName, notificationChannel, checked));
+  }, [dispatch, groupId, preferenceName]);


no need to pass dispatch here, instead pass updatePreferenceValue

updatePreferenceValue is a function. I don't think we need to pass function in dependency list. Added dispatch to avoid disabling lint for this line

sundasnoreen12 · 2023-05-13T20:09:16Z

src/notification-preferences/NotificationPreferences.jsx

+  if (notificationStatus === LOADING_STATUS) {
+    return (
+      <div className="d-flex h-100">
+        <Spinner
+          variant="primary"
+          animation="border"
+          className="mx-auto my-auto"
+          style={{ width: '4rem', height: '4rem' }}
+        />
+      </div>
+    );
+  }


you can memo this like

const SpinnerComp = useMemo(() => {
if (notificationStatus === LOADING_STATUS) {
return (

<Spinner
variant="primary"
animation="border"
className="mx-auto my-auto"
style={{ width: '4rem', height: '4rem' }}
/>

);
}
return null;
}, [notificationStatus]);
if (SpinnerComp) { return SpinnerComp; }

Memoization not required here

sundasnoreen12 · 2023-05-13T20:10:50Z

src/notification-preferences/NotificationPreferences.jsx

+  );
+};
+
+export default NotificationPreferences;


memo this component
export default React.memo(NotificationPreferences);

NotificationPreferences component has no props. React.memo will not be of any use

sundasnoreen12 · 2023-05-13T20:12:00Z

src/notification-preferences/ToggleSwitch.jsx

+import PropTypes from 'prop-types';
+
+const ToggleSwitch = ({ value, onChange }) => (
+  <Form.Switch checked={value} onChange={(event) => onChange(event.target.checked)} />


add callback function for onChange

onChange is being passed as prop from NotificationPreferenceGroup and from NotificationPreferenceRow. These calls have been wrapped in useCallback there

src/notification-preferences/NotificationPreferenceGroup.jsx

awais-ansari · 2023-05-18T12:18:16Z

src/notification-preferences/NotificationPreferences.jsx

+  const preferenceGroups = useSelector(getPreferenceGroupIds());
+  const preferencesList = useMemo(() => (
+    preferenceGroups.map(key => (
+      <NotificationPreferenceGroup groupId={key} key={key} />
+    ))
+  ), [preferenceGroups]);
+  useEffect(() => {
+    dispatch(updateSelectedCourse(courseId));
+    if (courseStatus !== SUCCESS_STATUS) {


Please add newline between logics

muhammadadeeltajamul force-pushed the inf-863 branch from 1fe528c to ed8256a Compare May 4, 2023 07:01

muhammadadeeltajamul force-pushed the inf-863 branch from ed8256a to 5a32386 Compare May 8, 2023 00:39

muhammadadeeltajamul marked this pull request as ready for review May 8, 2023 00:48

muhammadadeeltajamul requested review from sundasnoreen12, ayesha-waris and awais-ansari May 8, 2023 00:49

muhammadadeeltajamul force-pushed the inf-863 branch from 5a32386 to b67fbb7 Compare May 9, 2023 06:11

sundasnoreen12 requested changes May 9, 2023

View reviewed changes

muhammadadeeltajamul force-pushed the inf-863 branch from b67fbb7 to 6eb8974 Compare May 9, 2023 06:54

muhammadadeeltajamul requested a review from sundasnoreen12 May 9, 2023 06:56

awais-ansari requested changes May 9, 2023

View reviewed changes

muhammadadeeltajamul force-pushed the inf-863 branch from 6eb8974 to 1282815 Compare May 12, 2023 11:46

sundasnoreen12 requested changes May 13, 2023

View reviewed changes

muhammadadeeltajamul force-pushed the inf-863 branch from 1282815 to 44a712b Compare May 15, 2023 01:09

sundasnoreen12 approved these changes May 16, 2023

View reviewed changes

awais-ansari requested changes May 18, 2023

View reviewed changes

feat: added notification preference ui

01f24b4

muhammadadeeltajamul force-pushed the inf-863 branch from 44a712b to 01f24b4 Compare May 19, 2023 11:09

awais-ansari approved these changes May 19, 2023

View reviewed changes

muhammadadeeltajamul merged commit 1927146 into master May 22, 2023

muhammadadeeltajamul deleted the inf-863 branch May 22, 2023 01:17

snglth pushed a commit to Abstract-Tech/community-theme-account that referenced this pull request Jan 9, 2024

feat: added notification preference ui (openedx#784)

5b7ccdb

This was referenced Feb 8, 2024

feat: Restore jump nav and content width #994

Open

feat: Restore jump nav and content width [backport from master] #995

Open

		// import { getConfig } from '@edx/frontend-platform';
		// import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth';

		// const url = `${getConfig().LMS_BASE_URL}/api/notifications/${courseId}`;
		// const { data } = await getAuthenticatedHttpClient().get(url);

		@@ -1,11 +1,16 @@
		/* eslint-disable import/no-named-default */

feat: added notification preference ui #784

feat: added notification preference ui #784

Conversation

muhammadadeeltajamul commented May 4, 2023 • edited Loading

codecov bot commented May 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammadadeeltajamul commented May 4, 2023 •

edited

Loading

codecov bot commented May 4, 2023 •

edited

Loading