Summary

Add new standard support for subforms on pages.

Review Request #10950 — Created March 12, 2020 and submitted March 14, 2020, 4:31 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

Commit

51021eb...

Reviewers

Groups

reviewboard

People

Description

We have a number of pages in the administration UI today that make use
of subforms, which are forms dynamically hidden or shown based on
the selection on another form control. Each one of these had to be
written manually, which made implementations inconsistent and sometimes
buggy.

This change introduces a new formal subform implementation, which we'll
be able to start using in the settings pages. There are several new
things introduced here:

An -is-subform modifier on rb-c-form-fieldset, which turns a
fieldset into a subform.
Data attributes for specifying a subform group, ID, and a controller
element ID (which is used to automatically manage subform displays).
Guidance on setting both the disabled and hidden attributes to
ensure the form's data won't be validated or sent to the server, and
hiding the form from display and from screen readers (note that
hidden is an HTML 5.1 attribute, which is available in all our
supported browsers, including IE11).
Support in RB.FormView for automatically setting up and managing
subform state based on the data attributes.
Standard templates for representing a form instance as a subform,
and a dictionary of subform IDs to forms as subforms.

Upcoming changes will update other forms to use this, fixing our form
validation issues on Django 1.11.

Testing Done

Unit tests pass.

Made use of this to update all the existing subforms (which will be
in an upcoming change). Verified everything worked as expected.

Issues

Description	From	Last Updated
Col: 48 ['group1'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 48 ['group2'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 41 ['group1'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 51 ['subform1'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 41 ['group1'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 51 ['subform2'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 41 ['group2'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 51 ['subform3'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 41 ['group2'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Col: 51 ['subform4'] is better written in dot notation.	reviewbot	March 12, 2020, 10:30 a.m.
Should be "Render the view"	david	March 13, 2020, 12:46 a.m.
Should this really be set to have a default, since options.group is required? Perhaps we should remove the default value …	david	March 13, 2020, 12:47 a.m.
controllerGroup?	david	March 13, 2020, 12:47 a.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 48
 ['group1'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 48
 ['group2'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 ['group1'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 51
 ['subform1'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 ['group1'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 51
 ['subform2'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 ['group2'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 51
 ['subform3'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 41
 ['group2'] is better written in dot notation.
```
reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 51
 ['subform4'] is better written in dot notation.
```

Change Summary:

Fixed Review Bot complaints.

Commit:

5f17f4b90455353cac84c533ce6410c9da9c8163

797b0688e3b80b5d3abfb4266ea3d55826e7ded5

Diff:

Revision 2 (+417 -7)

Show changes

	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/ui/views/formView.es6.js
	reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js
	reviewboard/templates/forms/subform.html
	reviewboard/templates/forms/subforms.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be "Render the view"
```

reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 2)

The issue has been resolved. Show all issues

Should this really be set to have a default, since options.group is required? Perhaps we should remove the default value and then assert on options?

chipx86

March 13, 2020, 12:47 a.m.

I was just aiming to make the errors fully about the parameters (if you provide a blank options, you're not doing any better), but I can change that.

reviewboard/static/rb/js/ui/views/formView.es6.js (Diff revision 2)
The issue has been resolved. Show all issues
```
controllerGroup?
```
1. chipx86
  
  March 13, 2020, 12:47 a.m.
  Oops, search/replace issue. Was changing "category" to "group". Missed fixing this one.

Change Summary:


Fixed a doc comment.
Fixed a variable name due to a search/replace.
Removed the default for options in setSubformVisibility() and added an assertion.

Commit:

797b0688e3b80b5d3abfb4266ea3d55826e7ded5

51021eb9cc1eb2e49d2bc39ae8dc4fe4ef1fed0c

Diff:

Revision 3 (+420 -7)

Show changes

	reviewboard/static/rb/css/ui/forms.less
	reviewboard/static/rb/js/ui/views/formView.es6.js
	reviewboard/static/rb/js/ui/views/tests/formViewTests.es6.js
	reviewboard/templates/forms/subform.html
	reviewboard/templates/forms/subforms.html

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-4.0.x (ea878e6)