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.

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: hide 2U specific fields #797

Closed
wants to merge 1 commit into from
Closed

Conversation

uzairr
Copy link
Contributor

@uzairr uzairr commented Nov 22, 2022

Few 2U specific fields are appearing for all types of the course(s) which is creating confusion for the partners.Due to this change, 2U fields would only be displayed for 2U courses

PROD-3032

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Can we please make this a configurable setting, instead of making hard-coded references to 2U in the code?

Few 2U specific fields are appearing for all types of the course(s)
which is creating confusion for the partners.Due to this change,
2U fields would only be displayed for 2U courses

PROD-3032
@uzairr uzairr force-pushed the hide-2u-specific-fields branch from 801ab61 to a9aa6fb Compare November 23, 2022 06:25
Copy link
Contributor

@Ali-D-Akbar Ali-D-Akbar left a comment

Choose a reason for hiding this comment

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

Do we have a snapshot where these override fields appear?
If yes then it looks good to me.

Copy link
Contributor

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

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

Still think we can make this more generic. Thanks for bearing with me.

@@ -226,6 +227,10 @@ export class BaseEditCourseForm extends React.Component {
this.setCollapsible(true);
}

filterCourseTypes2U(courseType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove "2U" from the object and parameter names we can avoid confusion in the future, making this a truly generic feature.

Suggested change
filterCourseTypes2U(courseType) {
filterCourseTypes(courseType) {

@@ -306,6 +311,7 @@ export class BaseEditCourseForm extends React.Component {
priceLabels,
runTypeModes,
} = parsedTypeOptions;
const courseTypes2U = courseTypeOptions.filter(this.filterCourseTypes2U);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const courseTypes2U = courseTypeOptions.filter(this.filterCourseTypes2U);
const courseTypesToFilter = courseTypeOptions.filter(this.filterCourseTypes);

<FieldLabel
id="organization_logo_override.label"
text="Organization Logo Override"
{courseTypes2U?.length > 0 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{courseTypes2U?.length > 0 && (
{courseTypesToFilter?.length > 0 && (

@@ -1,4 +1,5 @@
NODE_ENV='production'
IDENTIFIER_2U='2U'
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rename this as well, and remove the "2U" default setting which does not make sense for all use cases.

Suggested change
IDENTIFIER_2U='2U'
COURSE_TYPE_FILTER=''

@@ -1,4 +1,5 @@
NODE_ENV='development'
IDENTIFIER_2U='2U'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IDENTIFIER_2U='2U'
COURSE_TYPE_FILTER=''

@@ -226,6 +227,10 @@ export class BaseEditCourseForm extends React.Component {
this.setCollapsible(true);
}

filterCourseTypes2U(courseType) {
return courseType.label.includes(getConfig().IDENTIFIER_2U);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return courseType.label.includes(getConfig().IDENTIFIER_2U);
return courseType.label.includes(getConfig().COURSE_TYPE_FILTER);

@@ -5,3 +5,4 @@ LOGO_TRADEMARK_URL=https://edx-cdn.org/v3/default/logo-trademark.svg
LOGO_WHITE_URL=https://edx-cdn.org/v3/default/logo-white.svg
FAVICON_URL=https://edx-cdn.org/v3/default/favicon.ico
SITE_NAME='edX'
IDENTIFIER_2U='2U'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IDENTIFIER_2U='2U'
COURSE_TYPE_FILTER=''

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants