Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ temp/babel-plugin-react-intl
### pyenv ###
.python-version

### vim ###

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Respect :)

*.swp

### Emacs ###
*~
/temp
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@fortawesome/free-solid-svg-icons": "5.11.2",
"@fortawesome/react-fontawesome": "0.1.9",
"babel-polyfill": "6.26.0",
"classnames": "^2.2.6",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

edit: I see @xitij2000 originally suggested this approach, though the relevant lines were much shorter then, but happy to discuss.

Tagging @davidjoy since you're often a firewall against new dependencies (which is a good thing!).

Do we use this library in other frontend-apps?

It's not a big library, but it still feels like a heavy integration for what is effectively "append one class to the list based on a boolean".

From below:

return (
  <div className={classNames('course-page-config-card d-flex flex-column align-content-stretch bg-white p-3 border shadow', { 'border-info-300': coursePage.isEnabled, 'border-gray-100': !coursePage.isEnabled })}>
  // ... 

could be simplified (without dependency) as:

let classNames = 'course-page-config-card d-flex flex-column align-content-stretch bg-white p-3 border shadow';
if (coursePage.isEnabled) {
  classNames += ' border-info-300';
} else {
  classNames += ' border-gray-100;
}
return (
  <div className={classNames}>
  // ...

or maybe even:

let extraClassNames;
if (coursePage.isEnabled) {
  extraClassNames = 'border-info-300';
} else {
  extraClassNames = 'border-gray-100;
}
return (
  <div className={`${extraClassNames} course-page-config-card d-flex flex-column align-content-stretch bg-white p-3 border shadow`}>
  // ...

Comparing the example and counter-examples, beyond the dependency issue itself, my other concern is with just how long the line is, and in particular, that the important parts (the conditionals) are tucked in at the end.

Important code that happens off-screen tends to be more likely to introduce issues/regressions, as it is easy for both author/reviewer/debugger to overlook mistakes; it's literally harder to see.

I don't mean to push for 80 character widths or anything (I mean, I do want to, but I wont; and I think 120 is fairly standard in the edx ecosystem), but this clocks in at 210 (!?) characters, with the critical bits happening off-screen.

Python's ternary operator should also be avoided, at least in verbose situations, for the same reasoning:

my_variable = the_result_of_some_computed_value('with', 'arguments' and 'stuff').trim().lower()) if CONDITION_THAT_RUNS_SO_FAR_OFF_THE_SCREEN_AND_CANT_BE_EASILY_SEEN else IT_IS_MORE_LIKELY_TO_CAUSE_BUGS

Yes, that code is contrived, but yes, this issue does bite us/me in real life, including the first time I read this PR and missed that line 😉

So unless @davidjoy is strongly in favor of this library (and we can still refactor to shrink lines), I'd prefer to remove this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and if we were to keep this, is there a package-lock.json that should be committed too?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we use this library in other frontend-apps?

Yes, I think it is used in almost all other frontend app, and in edx-platform itself. While I had prior experience with it, I did look into it before suggesting it here.

Even if we'd like to avoid another dependency (npm dependencies can quickly get unwieldy), I think a helper for this is a pretty good thing to include in the frontend-platform.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unless @davidjoy protests, I'm fine with this library then, thanks for checking.

If we could though, still refactor its usage to break up the long line(s) so the conditional logic isn't hidden off-screen?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, we use classnames in many apps and have found it really useful. 👍 It shorthands a lot of complicated logic very nicely.

"email-validator": "^2.0.4",
"moment": "^2.27.0",
"prop-types": "15.7.2",
Expand Down
118 changes: 118 additions & 0 deletions src/course-page-resources/CoursePageResources.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import PropTypes from 'prop-types';
import React, { useContext } from 'react';
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { AppContext } from '@edx/frontend-platform/react';

import CoursePageConfigCard from './course-page/CoursePageConfigCard';
import messages from './messages';

// XXX this is just for testing and should be removed ASAP
const coursePages = [
{
id: 'cp-discussion',
title: 'Discussion',
isEnabled: false,
showSettings: false,
showStatus: false,
showEnable: true,
description: 'Encourage participation and engagement in your course with discussion forums',
},
{
id: 'cp-teams',
title: 'Teams',
isEnabled: true,
showSettings: true,
showStatus: true,
showEnable: false,
description: 'Leverage teams to allow learners to connect by topic of interest',
},
{
id: 'cp-progress',
title: 'Progress',
isEnabled: false,
showSettings: true,
showStatus: true,
showEnable: false,
description: 'Allow students to track their progress throughout the course lorem ipsum',
},
{
id: 'cp-textbooks',
title: 'Textbooks',
isEnabled: true,
showSettings: true,
showStatus: true,
showEnable: false,
description: 'Provide links to applicable resources for your course',
},
{
id: 'cp-notes',
title: 'Notes',
isEnabled: true,
showSettings: true,
showStatus: true,
showEnable: false,
description: 'Support individual note taking that is visible only to the students',
},
{
id: 'cp-wiki',
title: 'Wiki',
isEnabled: false,
showSettings: false,
showStatus: false,
showEnable: true,
description: 'Share your wiki content to provide additional course material',
},
];

function CoursePageResources({ intl, courseId }) {
const { config } = useContext(AppContext);
const lmsCourseURL = `${config.LMS_BASE_URL}/courses/${courseId}`;
return (
<main>
<div className="container-fluid bg-info-100">
<div className="d-flex justify-content-between align-items-center border-bottom">
<h1 className="mt-3 text-info-500">{intl.formatMessage(messages.heading)}</h1>
<a className="btn btn-primary" href={lmsCourseURL} role="button">
{intl.formatMessage(messages['viewLive.button'])}
</a>
</div>
<div className="text-info-500">
<h3 className="mt-3">
{intl.formatMessage(messages['pages.subheading'])}
</h3>
<div className="d-flex flex-wrap align-items-stretch justify-content-around">
{coursePages.map((coursePage) => (
<div
className="d-flex flex-column align-content-stretch p-3 col-sm-12 col-md-6 col-lg-4"
key={coursePage.id}
>
<CoursePageConfigCard coursePage={coursePage} />
</div>
))}
</div>
</div>
<div>
<h3 className="text-info-500">{intl.formatMessage(messages['resources.subheading'])}</h3>
<div className="row bg-white text-info-500 border shadow justify-content-center align-items-center my-3 mx-1">
<div className="col-1 font-weight-bold">{intl.formatMessage(messages['resources.custom.title'])}</div>
<div className="col-8 my-3">
{intl.formatMessage(messages['resources.custom.description'])}
</div>
<div className="col-2 text-right">
<a className="btn btn-outline-info" href="/#" role="button">
{intl.formatMessage(messages['resources.newPage.button'])}
</a>
</div>
</div>
</div>
</div>
</main>
);
}

CoursePageResources.propTypes = {
intl: intlShape.isRequired,
courseId: PropTypes.string.isRequired,
};

export default injectIntl(CoursePageResources);
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
describe('example', () => {
describe('coursepageresources', () => {
it('will pass because it is an example', () => {

});
Expand Down
62 changes: 62 additions & 0 deletions src/course-page-resources/course-page/CoursePageConfigCard.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import PropTypes from 'prop-types';
import React from 'react';
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
import { faCog } from '@fortawesome/free-solid-svg-icons';
import { Button } from '@edx/paragon';

import classNames from 'classnames';
import messages from '../messages';

const CoursePageShape = PropTypes.shape({
id: PropTypes.string.isRequired,
title: PropTypes.string.isRequired,
description: PropTypes.string.isRequired,
isEnabled: PropTypes.bool.isRequired,
showSettings: PropTypes.bool.isRequired,
showStatus: PropTypes.bool.isRequired,
showEnable: PropTypes.bool.isRequired,
});

export { CoursePageShape };

function CoursePageConfigCard({ intl, coursePage }) {
const pageStatusMsgId = coursePage.isEnabled ? 'pageStatus.enabled' : 'pageStatus.disabled';
const componentClasses = classNames(
'course-page-config-card d-flex flex-column align-content-stretch',
'bg-white p-3 border shadow',
{ 'border-info-300': coursePage.isEnabled, 'border-gray-100': !coursePage.isEnabled },
);

return (
<div className={componentClasses}>
<div className="d-flex flex-row">
<span className="font-weight-bold">{coursePage.title}</span>
{coursePage.showSettings && <FontAwesomeIcon icon={faCog} className="ml-auto" />}
</div>

<div>
{coursePage.showStatus && <span>{intl.formatMessage(messages[pageStatusMsgId])}</span>}
</div>

<div className="mt-3">
<p>{coursePage.description}</p>
</div>

{coursePage.showEnable && !coursePage.isEnabled && (
<div className="d-flex justify-content-center">
<Button className="btn btn-outline-primary">
{intl.formatMessage(messages['enable.button'])}
</Button>
</div>
)}
</div>
);
}

CoursePageConfigCard.propTypes = {
intl: intlShape.isRequired,
coursePage: CoursePageShape.isRequired,
};

export default injectIntl(CoursePageConfigCard);
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
describe('CoursePageConfigCard', () => {
it('will pass because it is an example', () => {

});
});
File renamed without changes.
File renamed without changes.
2 changes: 2 additions & 0 deletions src/course-page-resources/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/* eslint-disable import/prefer-default-export */
export { default as CoursePageResources } from './CoursePageResources';
3 changes: 3 additions & 0 deletions src/course-page-resources/index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.course-page-config-card {
flex-basis: 100%;
Comment thread
stvstnfrd marked this conversation as resolved.
}
46 changes: 46 additions & 0 deletions src/course-page-resources/messages.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { defineMessages } from '@edx/frontend-platform/i18n';

const messages = defineMessages({
heading: {
id: 'course-authoring.pages-resources.heading',
defaultMessage: 'Pages & Resources',
},
'viewLive.button': {
id: 'course-authoring.pages-resources.viewLive.button',
defaultMessage: 'View Live',
},
'enable.button': {
id: 'course-authoring.pages-resources.enable.button',
defaultMessage: 'Enable',
},
'pages.subheading': {
id: 'course-authoring.pages-resources.pages.subheading',
defaultMessage: 'Course pages',
},
'pageStatus.enabled': {
id: 'course-authoring.pages-resources.pageStatus.enabled',
defaultMessage: 'Enabled',
},
'pageStatus.disabled': {
id: 'course-authoring.pages-resources.pageStatus.disabled',
defaultMessage: 'Disabled',
},
'resources.subheading': {
id: 'course-authoring.pages-resources.resources.subheading',
defaultMessage: 'Resources',
},
'resources.custom.title': {
id: 'course-authoring.pages-resources.resources.custom.title',
defaultMessage: 'Custom',
},
'resources.custom.description': {
id: 'course-authoring.pages-resources.resources.custom.description',
defaultMessage: 'Create and edit custom pages to provide students with additional course content and resources. Pages are publicly visible. If users know the URL of a page, they can view the page even if they are not registered for or logged in to your course.',
},
'resources.newPage.button': {
id: 'course-authoring.pages-resources.resources.newPage.button',
defaultMessage: 'New Page',
},
});

export default messages;
18 changes: 0 additions & 18 deletions src/example/ExamplePage.jsx

This file was deleted.

Empty file removed src/example/index.scss
Empty file.
21 changes: 15 additions & 6 deletions src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import React from 'react';
import ReactDOM from 'react-dom';
import { Route, Switch } from 'react-router-dom';

import Header, { messages as headerMessages } from '@edx/frontend-component-header';
import { messages as headerMessages } from '@edx/frontend-component-header';
import Footer, { messages as footerMessages } from '@edx/frontend-component-footer';

import appMessages from './i18n';
import ExamplePage from './example/ExamplePage';
import { CoursePageResources } from './course-page-resources';
import ProctoredExamSettings from './proctored-exam-settings/ProctoredExamSettings';
import StudioHeader from './studio-header/Header';

Expand All @@ -36,10 +36,19 @@ subscribe(APP_READY, () => {
);
}}
/>
<Route path="/example">
<Header />
<ExamplePage />
</Route>
<Route
path="/course-pages/:course_id"
render={({ match }) => {
const courseId = decodeURIComponent(match.params.course_id);
return (
<>
<StudioHeader courseId={courseId} />
<CoursePageResources courseId={courseId} />
<Footer />
</>
);
}}
/>
</Switch>
<Footer />
</AppProvider>,
Expand Down
3 changes: 3 additions & 0 deletions src/index.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
@import '~@edx/paragon/scss/edx/theme.scss';

@import './course-page-resources/index.scss';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See above.


@import "~@edx/frontend-component-header/dist/index";
@import "~@edx/frontend-component-footer/dist/footer";

@import "proctored-exam-settings/proctoredExamSettings.scss";
Expand Down