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

Form Layouts #1

Merged
merged 16 commits into from
Jul 11, 2018
Merged

Form Layouts #1

merged 16 commits into from
Jul 11, 2018

Conversation

berdyshev
Copy link
Contributor

Here is the FieldLayout component with a possibility to define fields config and variations (dependent fields) in a simple javascript structure.

  • <FieldLayout /> (src/Components/FieldLayout.jsx) doesn't provide an entire form and could be used as a part of the form.
  • All "business" logic is extracted into separate modules (src/layouts/) to leave only rendering-related code in the FieldLayout component and this allowed to cover this logic with unit tests (see tests/jest/Layout/layout.js).
  • There are 6 comparison operators defined: eq, gt (greater than), gte (greater or equal), lt (less than), lte (less or equal), in (value is in array of allowed values)
  • Supported field components are
    const components = {
        file:        FileUpload,
        text:        Text,
        textarea:    Textarea,
        date:        DatePicker,
        datetime:    DateTimePicker,
        checkbox:    Checkboxes,
        choice:      DropDown,
        radio:       Radio,
        multichoice: MultipleDropDown
    };
  • There are 7 validation functions are defined required, min, max, minLength, maxLength, regex, dateRange with possibility to pass parameters into them
  • Existed ticket_form storybook is completely reproduced with a new FieldLayout functionality (with a small bonus — now form values are not reseted once the layout changed)

@coveralls
Copy link

coveralls commented Jul 10, 2018

Pull Request Test Coverage Report for Build 43

  • 64 of 141 (45.39%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+10.4%) to 31.358%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Components/Field.jsx 3 4 75.0%
src/layouts/Layout.js 21 22 95.45%
src/layouts/validations.js 39 44 88.64%
src/layouts/conditions.js 1 6 16.67%
src/Components/FieldLayout.jsx 0 31 0.0%
src/Components/TicketForm.jsx 0 34 0.0%
Files with Coverage Reduction New Missed Lines %
src/Components/TicketForm.jsx 2 0.0%
Totals Coverage Status
Change from base Build 38: 10.4%
Covered Lines: 152
Relevant Lines: 479

💛 - Coveralls

@chroder chroder requested a review from jducro July 11, 2018 08:09
@@ -0,0 +1,364 @@
export default [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would move this file into /tests/DemoState as it is only needed for tests and storybook and would need to be provided when using this library

package.json Outdated
@@ -8,9 +8,11 @@
"@deskpro/js-utils": "^1.0.4",
"@deskpro/react-datepicker-hijri": "^1.4.2-beta.1",
"@storybook/addon-knobs": "^3.4.3",
"ajv": "^6.5.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

That should be a dev dependency, it is not used outside the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had an idea to use this validation in the LayoutConfig helper in order to warn developers that the layout is not valid. But the library is a bit heavy so I decided to not implement that. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably better to keep things lightweight. I'm already worried by moment size ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can replace moment with date-fns...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will look into that. Does it support hijri calendar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I don't know..

@berdyshev
Copy link
Contributor Author

@jducro I have fixed two your comments

@jducro jducro merged commit d1576e5 into master Jul 11, 2018
@jducro jducro deleted the feature/form-schema branch July 11, 2018 11:39
jducro added a commit that referenced this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants