[BD-03] [BB-2542] ]Add discussions configuration UI wireframe in MFE #2

rusrushal13 · 2020-06-26T00:33:54Z

Wireframe for the discussion configurations, currently work in progress

JIRA tickets:

BB-2542

Discussions:

Dependencies:

Mockups: https://edx.invisionapp.com/share/XDXMC3XU9MR#/screens/420742420

Screenshots:

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline:

Testing instructions:

Author notes and concerns:

Work is in progress

Reviewers

@xitij2000
edX reviewer[s] TBD

openedx-webhooks · 2020-06-26T00:34:09Z

Thanks for the pull request, @rusrushal13! I've created BLENDED-371 to keep track of it in Jira.

When this pull request is ready, tag your edX technical lead.

marcotuts · 2020-06-29T16:35:03Z

Looking great! I will flag this to the UX team who can provide final visual details on this next. Thanks!

xitij2000 · 2020-06-30T07:57:14Z

src/CoursePageResources/CoursePageResources.jsx

+    <main>
+      <div className="container-fluid">
+        <div className="container-fluid-container d-flex justify-content-between align-items-center border-bottom">
+          <h1 className="container-fluid-heading mt-3">Pages & Resources</h1>


All text like this should be translated not directly entered here.
The process for that is roughly as follows:

Create a messages.js file in the same directory. You can see a random example I picked here: https://github.com/edx/frontend-app-learning/blob/master/src/courseware/course/celebration/messages.js
-- You can follow the same exact format, it is an object of unique keys mapped to different messages.
-- Each message has an id that is globally unique, and at the very least, a defaultMesage, this is the text that would actually be placed here.
-- It can also contain a description, this can be helpful to translators to get some context about where the word is used so they can pick the most appropriate translation.

Next import this file.

The intl object will already be injected into the component since the component is wrapped with injectIntl. So you can use intl.formatMessage(message.key) wherever you'd want to use some text. Here is an example: https://github.com/edx/frontend-app-learning/blob/a6edc9132f648890eb5693e644998174894fbe41/src/courseware/course/celebration/CelebrationModal.jsx#L34

In this case here is how I would do it:

messages.js

import { defineMessages } from '@edx/frontend-platform/i18n'; const messages = defineMessages({ heading: { id: 'course-authoring.pages-resources.heading`, defaultMessage: 'Pages & Resourses' } ... }

The "View Live" button, the "Course Pages" subheading etc all should be in the messages file.

xitij2000 · 2020-06-30T07:59:06Z

src/CoursePageResources/CoursePageResources.jsx

+          <h1 className="container-fluid-heading mt-3">Pages & Resources</h1>
+          <Button className="container-fluid-button font-weight-bold rounded my-3">
+            View Live
+          </Button>


This is a link masquerading as a button, so you can just use a link button styling in bootstrap.

Using the following, I get a rounded button instead of the rectangular button in the wireframe. What should I do?

<a class="btn btn-primary" href="#" role="button"> {intl.formatMessage(messages['course-authoring.pages-resources.viewLive.button'])} </a>

@taniwha Feel free to just leave it as is for right now.
We'll have the UI folks run over the details later to confirm expectations.

xitij2000 · 2020-06-30T08:00:00Z

src/CoursePageResources/CoursePageResources.jsx

+                enabledButton
+                coursePageStatus={false}
+                description="Encourage Participation and engagement in your course with discussion forums"
+              />


This should not be hard-coded, it will be constructed based on data from the API.

I assume you mean cousePageStatus.

Note: As I mentioned above, these tiles should not be hard-coded, they will be returned by an API. So you did not need to move all this to the translation file. i.e. Remove all <CoursePageConfigTile objects from here and have this list be provided by the parent element for CoursePageResources

xitij2000 · 2020-06-30T08:00:31Z

src/CoursePageResources/CoursePageResources.test.jsx

@@ -1,4 +1,4 @@
-describe('example', () => {
+describe('coursepageresources', () => {
  it('will pass because it is an example', () => {

  });


Add some snapshot tests.

Note - we'd like to avoid overuse of snapshot tests, as while they're easy to write, they're brittle and difficult to maintain. Favoring specific assertions about the DOM is preferable, as it's much less likely to break when trivial changes are made to the markup/CSS classnames.

Snapshots are good for small components, but not great for larger components. i.e., the cost of ensuring that the snapshot is always 100% correct is so high for large components that it's very easy for the snapshot to become invalid, in which case the test starts asserting that things are still wrong, but no one realizes because it passes. :)

A large, invalid snapshot is very difficult to go back and prove wrong because it requires a developer to go understand every little thing that's in the snapshot, and to go against what the machine is telling them - that the test passed. Visual inconsistencies are better caught with our eyes when viewing the UI, rather than trying to parse a not-very-human-readable HTML document.

Some good rationale in here: https://engineering.ezcater.com/the-case-against-react-snapshot-testing

Thanks for this explanation! Definitely useful to keep in mind for future PRs as well.

What are snapshot tests? Or maybe better, can you point me to some examples? Will I find some in frontend-app-learning?

Just read David Joy's comment (my page was out of date) and I'm inclined to agree that meaning is more important than appearance (to paraphrase).

Snapshot tests record a copy of the DOM generated from react when they're first run, and then on subsequent runs compare the results to that original snapshot. It's like asserting that the entire DOM is what you expect it to be, down to every single tag and attribute. As mentioned above, this means that any tiny change no matter how trivial invalidates a snapshot test and can make noisy diffs - often this can result in masking real issues if the test is "retrained" under faulty assumptions about what's correct.

You can see some in this commit in frontend-app-learning: openedx/frontend-app-learning@ada0750

Note that I've asked that those be modified (in the same spirit as above), so that code may not make it into master. :)

Yeah, I read the rational and couldn't agree more. I had done similar by hand years ago doing compiler testing and hated it, thus avoided doing automated tests until I figured out how to do meaningful tests. More work to write the tests, but they are very much more reliable.

xitij2000 · 2020-06-30T08:06:56Z

src/CoursePageResources/index.scss

@@ -0,0 +1,48 @@
+.container-fluid {
+    background-color: #EAF3FB;
+    color: #2E8ED3;


Colours shouldn't be hard-coded. Get them from the theme, or at least set them globally in a theme file.

xitij2000 · 2020-06-30T08:16:56Z

src/CoursePageResources/index.scss

+.border-enabled {
+    border-color: #4293C4 !important; 
+    border-width: 2px;
+    border-style: solid;
+    border-radius: 5px;
+}
+
+.border-disabled {
+    border-color: #ffffff !important;
+    border-width: 2px;
+    border-style: solid;
+    border-radius: 5px;
+}


Both of these share most of their code. It would be better to have it as:

.course-page-tile { border: 2px solid $white; border-radius: 5px; //Maybe this can be standardised? .disabled { border-coloor: #4293C4; } } }

xitij2000 · 2020-06-30T08:19:41Z

src/CoursePageResources/index.scss

+.container-fluid-container {
+    border-color: #B2D5EE;
+}
+
+.container-fluid-enabled-button {
+    border-color: #0075B4;
+    color: #2E8ED3;
+}
+
+.container-fluid-enabled-button:hover {
+    color: inherit;
+}
+
+.sub-container-text {
+    color: #39AAE3;
+}
+


These names are very generic and tied to bootstraps naming scheme instead of being specific to this app.

xitij2000 · 2020-06-30T08:24:46Z

src/CoursePageResources/index.scss

+.container-fluid-button {
+    background-color: #0075B4;
+    color: #ffffff;
+    font-size: 12px;
+    &:hover {
+        color: #ffffff;
+    }
+}


This is a very generic name for this button, and again uses the bootstrap names instead of something like course-pages-button or something.
Other points:

Colours should be in the theme, not here.

Font size should be in rem not pixels.

xitij2000 · 2020-06-30T08:25:37Z

src/course-page/CoursePage.jsx

+import { faCog } from '@fortawesome/free-solid-svg-icons';
+import { Button } from '@edx/paragon';
+
+function CoursePage({


I think this component name is a bit misleading. A better name I feel would be: CoursePageConfigTile.

I believe these are generally called "Cards" by both Bootstrap and the edX UX team: https://getbootstrap.com/docs/4.0/components/card/

Should I rename the file as well? What about the directory? The former seems very reasonable, but I am unsure about the latter.

I think it's a virtue to keep naming consistent... so if we think these are 'cards' instead of 'pages', then I'd consistently change the term, yeah. Assuming that the course-page module is all about code related to these cards.

It seems to be about the cards, and I'll go with that naming convention. Thank you.

@taniwha As mentioned above in the comment, the convention here is to call these UI elements Cards, not Tiles. So could you rename this accordingly?

xitij2000 · 2020-06-30T08:26:14Z

src/course-page/CoursePage.jsx

+    let classes = 'bg-white shadow p-3 m-3';
+    classes += coursePageStatus ? ' border-enabled' : ' border-disabled';
+    return classes;
+  };


Instead of this, use the classNames utility. You can read more about that here, but to give a brief intro with that you can replace the above with:

classNames('course-page-tile bg-white shadow p-3 m-d', {'disabled': !coursePageStatus});

davidjoy · 2020-06-30T14:49:10Z

src/CoursePageResources/index.scss

@@ -0,0 +1,48 @@
+.container-fluid {


To agree with @xitij2000 's points below, we try to minimize semantic class names in our code to the bare minimum. Instead, we can use Bootstrap's utility classes to accomplish most of these things.

Paragon is built on top of Bootstrap, and provides a wealth of utility class names: https://edx.github.io/paragon/foundations/css-utilities

Also, as noted below, we definitely don't want to attach additional styling to existing Bootstrap class names; this will create very difficult to debug situations where unrelated code receive styling they're not expecting.

Thank you. I will look into that.

xitij2000 · 2020-07-06T09:05:14Z

src/CoursePageResources/messages.js

+import { defineMessages } from '@edx/frontend-platform/i18n';
+
+const messages = defineMessages({
+  'course-authoring.pages-resources.heading': {


nit: I think the idea is to give is a more friendly name here.

I think the 'id' needs to be globally unique, but this key in the dictionary does not need to be.
So I would recommend keeping them simple, so, 'heading' instead of 'course-authoring.pages-resources.heading'.

OK, I'll remove "course-authoring.pages-resources." from the dictionary keys but leave the id fields alone.

xitij2000 · 2020-07-06T09:07:35Z

src/CoursePageResources/index.scss

+  .disabled {
+    border-color: theme-color("gray", 100);
+  }
+}


I am wondering if this can all be standardised, and then this could just include the standard styling if it needed to be customised.

xitij2000 · 2020-07-06T09:11:06Z

src/course-page/CoursePage.jsx

    return classNames('bg-white shadow p-3 m-3', {
-        'border-disabled': !coursePageStatus,
-        'border-enabled': coursePageStatus,
+        'course-page-title.disabled': !coursePageStatus,
+        'course-page-title': coursePageStatus,
    });


This can be simplified to:

return classNames('bg-white shadow p-3 m-3 course-page-title', {disabled: !coursePageStatus});

That didn't work as intended because course-page-title.disabled is required, so I had to use a variation.

@taniwha that might be because you are defining the SCSS styling incorrectly.
Note:

.class1 { color: red; .class2 { color: green; } }

In the above the inner class2 will not match a div with both class1 and class2, It will match a child element of class2 inside an element with class1.

If you want to match an element that has both class1 and class2 (in this case both course-page-title and disabled), the correct scss code is:

.class1 { color: red; &.class2 { color: green; } }

xitij2000 · 2020-07-06T09:13:24Z

src/course-page/CoursePage.jsx

@@ -35,7 +35,7 @@ function CoursePageConfigTile({
      <p>{coursePageStatus ? 'Enabled' : 'Disabled'}</p>
      <div>{description}</div>
      <div className={getEnabledButtonClasses()}>


I don't see why we need to call an intermediary function here. Just add the entire classNames function call here.
If that makes it harder to understand, you can just put it in a variable. So something like:

const containerClasses = classNames('course-page-tile bg-white shadow p-3 m-d', {'disabled': !coursePageStatus}); ... // Later <div className={containerClasses} ...

xitij2000 · 2020-07-06T09:16:01Z

src/CoursePageResources/CoursePageResources.jsx

+                enabledButton
+                coursePageStatus={false}
+                description="Encourage Participation and engagement in your course with discussion forums"
+              />


Note: As I mentioned above, these tiles should not be hard-coded, they will be returned by an API. So you did not need to move all this to the translation file. i.e. Remove all <CoursePageConfigTile objects from here and have this list be provided by the parent element for CoursePageResources

xitij2000 · 2020-07-06T09:18:01Z

src/course-page/CoursePage.jsx

+import { faCog } from '@fortawesome/free-solid-svg-icons';
+import { Button } from '@edx/paragon';
+
+function CoursePage({


@taniwha As mentioned above in the comment, the convention here is to call these UI elements Cards, not Tiles. So could you rename this accordingly?

taniwha · 2020-07-06T10:48:49Z

@xitij2000 I had passed on the renaming for the moment because the user-view shows pages and wanted to query that, but I suppose it doesn't really matter if it's "page" externally and card internally (especially since it's a repleacable string).

taniwha · 2020-07-06T11:10:47Z

@xitij2000 I believe I have addressed all of your points other than the testing (not sure what to do there at this stage) and the API issue (thus some instances of page remain: they will be deleted when the API is sorted out). For that...
I take it that there would be no hard-coded cards and all card configs would be passed into CourseCardResources (hope I got the renaming intent right) in an array, and the cards put into the container in a for-loop. At this stage, I don:t know how to do this as I am actually not familiar with React or JavaScript (I've been getting along because things are similar enough to C that I can guess what's needed). What examples I can reference?
The same goes for the tests, actually: it is only unfamiliarity with appropriate testing methods that prevents me from implementing tests.

xitij2000 · 2020-07-08T05:47:26Z

I had passed on the renaming for the moment because the user-view shows pages and wanted to query that, but I suppose it doesn't really matter if it's "page" externally and card internally (especially since it's a repleacable string).

I think you misunderstood the intent behind the renaming. Just to clarify, we are building the UI for the "Course Pages" part of the app. So, it makes sense to call the internal component there: CoursePages.

The original name for the each individual component inside that Page (i.e. Discussion, Teams, Progress etc) was: CoursePage, which makes sense, but is confusing since a page can also be misunderstood to correspond to an actual "Course Page". Hence, I suggested that it be renamed to "CoursePageConfigTile" to clarify that it is a Tile that is for the CoursePage configuration. What David suggested is that instead of calling it a tile, we call it a Card, since that is the convention used by bootstrap, which is the UI styling toolkit we're using. i.e. the name of the component rendering each individual block with Discussions, Teams etc should be called "CoursePageCard" instead of the current "CourseCardConfigTile".

I take it that there would be no hard-coded cards and all card configs would be passed into CourseCardResources (hope I got the renaming intent right) in an array, and the cards put into the container in a for-loop. At this stage, I don:t know how to do this as I am actually not familiar with React or JavaScript (I've been getting along because things are similar enough to C that I can guess what's needed). What examples I can reference?

Sure, I can reach out about this on the internal Mattermost.

taniwha · 2020-07-08T05:50:31Z

I think you misunderstood the intent behind the renaming. Just to clarify, we are building the UI for the "Course Pages" part of the app. So, it makes sense to call the internal component there: CoursePages.

The original name for the each individual component inside that Page (i.e. Discussion, Teams, Progress etc) was: CoursePage, which makes sense, but is confusing since a page can also be misunderstood to correspond to an actual "Course Page". Hence, I suggested that it be renamed to "CoursePageConfigTile" to clarify that it is a Tile that is for the CoursePage configuration. What David suggested is that instead of calling it a tile, we call it a Card, since that is the convention used by bootstrap, which is the UI styling toolkit we're using. i.e. the name of the component rendering each individual block with Discussions, Teams etc should be called "CoursePageCard" instead of the current "CourseCardConfigTile".

I had indeed misunderstood. Thank you for the clarification. I will sort that out ASAP.

xitij2000

In my testing the style isn't matching the original mockup. Could you please make sure that it looks as close as possible without needing to add excessive overrides.

xitij2000 · 2020-07-10T10:34:38Z

src/course-page/CoursePageConfigCard.jsx

+      <div className="card-footer">
+        <div className={buttonClasses}>
+          <Button className="course-page-button bg-white border font-weight-bold rounded">
+            Enable


Move to messages file.

xitij2000 · 2020-07-10T10:34:56Z

src/course-page/CoursePageConfigCard.jsx

+        </div>
+      </div>
+      <div className="card-body">
+        <p>{isEnabled ? 'Enabled' : 'Disabled'}</p>


Move to messages file for translation.

xitij2000 · 2020-07-10T10:35:54Z

src/CoursePageResources/CoursePageResources.jsx

+  }].isRequired,
+};
+
+export default injectIntl(CoursePageResources);


Rename this directory to course-page-resources to stick with the approach everywhere else.

Also add an index.js file that re exports this component. i.e an index.js file that has:

export {default as CoursePageResources} from './CoursePageResources';

This inport this in the main index.jsx file as:

import {CoursePageResources} from './course-page-resources'

xitij2000 · 2020-07-10T10:36:57Z

src/course-page/CoursePageConfigCard.jsx

+  isEnabled: null,
+};
+
+export default injectIntl(CoursePageConfigCard);


This component is only going to be used by CoursePageResources, so move it to a subdirectory.

taniwha · 2020-07-13T05:06:09Z

@xitij2000 I've addressed your points and got the page looking much more like the wireframe. However, I noticed one issue that is probably a mistake on my part as I failed to notice it before I made my simplifications, but the wireframe has "Disabled" on some cards that do not have an "Enable" button: this currently does not make sense to me. Is this a mistake in the wireframe, or do I need to make the logic for the two items separate again?

[edit] question sorted in mattermost.

xitij2000 · 2020-07-14T14:45:15Z

src/course-page-resources/messages.js

+const messages = defineMessages({
+  heading: {
+    id: 'course-authoring.pages-resources.heading',
+    defaultMessage: 'Pages & Resourses',


nit: fix spelling

xitij2000 · 2020-07-14T14:46:32Z

src/course-page-resources/messages.js

+  },
+  '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 publicaly 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.',


nit: spelling fixes

xitij2000 · 2020-07-14T14:48:20Z

src/course-page-resources/index.scss

+.course-page-title {
+  border: 2px solid $white;
+  border-color: theme-color("info", 300);
+  border-radius: 5px; //Maybe this can be standardised?
+  &.disabled {
+    border-color: theme-color("gray", 100);
+  }
+}
+
+.course-page-button {
+  outline: 2px solid theme-color("info", 500);
+  outline-radius: 5px; //Maybe this can be standardised?
+  background-color: theme-color("gray", 100);
+  color: theme-color("info", 500);
+  font-size: 0.8;
+  &:hover {
+    color: inherit;
+  }
+}


I think these are hurting more than helping, just remove them. Let's get the basic structure in.

For now, I have left them in as they are what provide the specified shading.

xitij2000 · 2020-07-14T14:48:47Z

src/index.scss

@@ -1,6 +1,6 @@
 @import '~@edx/paragon/scss/edx/theme.scss';

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


Remove this as well.

xitij2000 · 2020-07-14T14:55:32Z