feat: shift course form error handling to @edx/paragon styles #805

zawan-ila · 2022-12-15T11:05:22Z

PROD-2734

Description

This PR updates the Publisher create course form to incorporate edx/paragon based validation for form fields

Testing Instructions

On the publisher landing page, click on New Course.
Fill in the details for this new course whilst intentionally missing some required fields.
Submit the form and observe the error messages.

ScreenShots

Before

After

ToDo

Update test snapshots

DawoudSheraz · 2022-12-20T11:49:33Z

src/components/DateTimeField/index.jsx

@@ -74,6 +74,10 @@ class DateTimeField extends React.Component {
      type,
      pattern,
      placeholder,
+      meta: {
+        touched,


why is touched needed here? Why cant the decision be done with error variable alone?

this is to prevent showing errors unnecessarily. Consider a form has just rendered with a required Datefield. Since the field is empty, the error prop will be true. However we do not wish to show it as an error, as the user has yet to interact with the field. To display errors properly, we need to keep track of whether the user has interacted with the field or not. For this we need the touched prop which will become active only if the user has touched the field e.g by clicking on it.

Ok. Is the prop coming from the react parent component?

ummm yeah... we are rendering it inside redux-form so we don't pass the meta prop explicitly but redux-form automagically does that for us

DawoudSheraz · 2022-12-20T11:50:37Z

src/utils/validation.js

+  if (pattern.test(value)) {
+    return undefined;
+  }
+  return 'Please Enter a valid course key';


DawoudSheraz · 2022-12-20T11:55:55Z

src/utils/validation.js

+  }
+
+  if (price < 1 || price > 1000) {
+    return 'Price should be between 1 and 1000(endpoints inclusive)';


nit: space before (

DawoudSheraz · 2022-12-20T11:56:08Z

src/utils/validation.js

+  const pattern = /^[0-9]+(\.)?[0-9]{0,2}$/;
+
+  if (!pattern.test(value)) {
+    return 'Please enter a valid price(with at most 2 decimal digits)';


nit: space before (

DawoudSheraz

I will let @AliAdnanSohail have a primary say on this since he had looked into this.

DawoudSheraz · 2022-12-28T06:10:37Z

src/components/CreateCoursePage/CreateCourseForm.jsx

@@ -101,17 +106,28 @@ class BaseCreateCourseForm extends React.Component {
      <div className="create-course-form">
        <h2>Create New Course</h2>
        <hr />
-        <form onSubmit={handleSubmit}>
+        <form onSubmit={handleSubmit} noValidate>


why is validation being removed when the form is submitted?

+1 to this? Validation to the form is a must-have.

well this is essentially the point of the PR....this removes the HTML(browser based) validation....if we don't do this then the browser will keep on showing its own tooltips on error fields....

we are doing the validations ourselves now...see the 'validate' prop passed to the Field components

DawoudSheraz · 2022-12-28T06:17:05Z

src/components/CreateCoursePage/index.jsx

@@ -35,6 +35,13 @@ class CreateCoursePage extends React.Component {
    this.setStartedFetching();
  }

+  componentDidUpdate(prevProps) {
+    // check if errors on form submission and scroll to them
+    if (this.props.courseInfo.error && (!prevProps.courseInfo || !prevProps.courseInfo.error)) {


This is checking that previous props did not have course info and error but current props have errors, correct?

DawoudSheraz · 2022-12-28T06:20:48Z

src/utils/validation.js

+const handleCourseCreateFail = (errors) => {
+  const fieldName = getFieldName(errors);
+  if (fieldName) {
+    const node = document.getElementsByName(fieldName)[0];
+
+    if (node) { node.scrollIntoView({ behavior: 'smooth' }); }
+  }
+};
+


shouldn't this live in CourseCreateForm?

handleCourseEditFail also lives in the same file so I though it would be better to follow the precedent

ansabgillani · 2022-12-28T21:24:52Z

src/components/CreateCoursePage/CreateCourseForm.jsx

@@ -101,17 +106,28 @@ class BaseCreateCourseForm extends React.Component {
      <div className="create-course-form">
        <h2>Create New Course</h2>
        <hr />
-        <form onSubmit={handleSubmit}>
+        <form onSubmit={handleSubmit} noValidate>


+1 to this? Validation to the form is a must-have.

src/components/CreateCoursePage/CreateCourseForm.jsx

ansabgillani · 2022-12-28T21:26:42Z

src/components/CreateCoursePage/CreateCourseForm.jsx

@@ -188,6 +216,11 @@ class BaseCreateCourseForm extends React.Component {
            <Field
              name="courseRunKey"
              component={RenderInputTextField}
+              props={


needed to make inline

ansabgillani · 2022-12-28T21:28:27Z

src/components/CreateCoursePage/index.jsx

@@ -164,6 +171,14 @@ class CreateCoursePage extends React.Component {
          { showForm
          && (
            <div>
+              {errorArray.length > 1 && (


What would errorArray contain if length is 1?

don't know...essentially this seems to be some sort of condition for displaying an error message....i have only changed the position of where the error message is being displayed....

if this ^ doesn't make sense, just look at the complete file and hopefully it will be clear

AliAdnanSohail

LGTM! Just few nits to address

AliAdnanSohail · 2022-12-30T08:45:04Z

src/utils/validation.js

+  if (pattern.test(value)) {
+    return undefined;
+  }
+  return 'Enter a valid course key';


nit: To make it consistent with other error messages, update this to Please enter a valid course key

AliAdnanSohail · 2022-12-30T08:47:15Z

src/components/PriceList/index.jsx

@@ -17,6 +21,11 @@ const PriceList = ({
        key={seatType}
        name={`prices.${seatType}`}
        component={RenderInputTextField}
+        props={


Suggested change

props={

props={{name: `prices.${seatType}`}}

zawan-ila changed the title ~~feat: shift course form error handling and submission from html-based to js-based(in particular following edx/paragon styles)~~ feat: shift course form error handling to @edx/paragon styles Dec 16, 2022

zawan-ila force-pushed the anawaz/prod2733/part2 branch 3 times, most recently from 7513e46 to fe90c5c Compare December 19, 2022 08:49

DawoudSheraz requested review from DawoudSheraz, AliAdnanSohail and ansabgillani December 19, 2022 13:41

DawoudSheraz reviewed Dec 20, 2022

View reviewed changes

DawoudSheraz reviewed Dec 28, 2022

View reviewed changes

ansabgillani suggested changes Dec 28, 2022

View reviewed changes

ansabgillani approved these changes Dec 30, 2022

View reviewed changes

AliAdnanSohail approved these changes Dec 30, 2022

View reviewed changes

zawan-ila force-pushed the anawaz/prod2733/part2 branch 2 times, most recently from 6d1d2a0 to ae6a40b Compare January 13, 2023 11:27

feat: shift new course form error handling to @edx/paragon styles

a29ff37

zawan-ila force-pushed the anawaz/prod2733/part2 branch from ae6a40b to a29ff37 Compare January 13, 2023 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: shift course form error handling to @edx/paragon styles #805

feat: shift course form error handling to @edx/paragon styles #805

zawan-ila commented Dec 15, 2022 •

edited

Loading

DawoudSheraz Dec 20, 2022

zawan-ila Dec 27, 2022

DawoudSheraz Dec 28, 2022

zawan-ila Dec 29, 2022

DawoudSheraz Dec 20, 2022

zawan-ila Dec 27, 2022

DawoudSheraz Dec 20, 2022

zawan-ila Dec 27, 2022

DawoudSheraz Dec 20, 2022

zawan-ila Dec 27, 2022

DawoudSheraz left a comment

DawoudSheraz Dec 28, 2022

ansabgillani Dec 28, 2022

zawan-ila Dec 29, 2022 •

edited

Loading

DawoudSheraz Dec 28, 2022

zawan-ila Dec 29, 2022

DawoudSheraz Dec 28, 2022 •

edited

Loading

zawan-ila Dec 29, 2022

ansabgillani Dec 28, 2022

ansabgillani Dec 28, 2022

zawan-ila Dec 29, 2022

ansabgillani Dec 28, 2022

zawan-ila Dec 29, 2022

AliAdnanSohail left a comment

AliAdnanSohail Dec 30, 2022

zawan-ila Jan 13, 2023

AliAdnanSohail Dec 30, 2022

zawan-ila Jan 13, 2023

feat: shift course form error handling to @edx/paragon styles #805

Are you sure you want to change the base?

feat: shift course form error handling to @edx/paragon styles #805

Conversation

zawan-ila commented Dec 15, 2022 • edited Loading

Description

Testing Instructions

ScreenShots

Before

After

ToDo

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

DawoudSheraz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zawan-ila Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudSheraz Dec 28, 2022 • edited Loading

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

AliAdnanSohail left a 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

zawan-ila commented Dec 15, 2022 •

edited

Loading

zawan-ila Dec 29, 2022 •

edited

Loading

DawoudSheraz Dec 28, 2022 •

edited

Loading