feat(Form): initial Implementation #242

andybessm · 2019-12-09T08:16:36Z

Thank you for your contribution! 👏

To get it merged faster, kindly review the checklist below:

Pull Request Checklist

Reviewed the Contributing Guidelines
- Especially the How to Contribute section
Correct commit message style

…lytical-table-dnd

…use of a standard style added; custom dnd hook added

… into feat/analytical-table-dnd

… with absolute layout BREAKING CHANGE: Update `react-table` to `7.0.0-rc.23`

… into feat/form-component

…m-component

netlify · 2019-12-09T08:29:04Z

Deploy preview for ui5-webcomponents-react ready!

Built with commit 211ac8f

https://deploy-preview-242--ui5-webcomponents-react.netlify.com

codecov-io · 2019-12-09T10:32:49Z

Codecov Report

Merging #242 into master will decrease coverage by 0.04%.
The diff coverage is 69%.

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
- Coverage   72.53%   72.48%   -0.05%     
==========================================
  Files         156      162       +6     
  Lines        3466     3558      +92     
  Branches      605      618      +13     
==========================================
+ Hits         2514     2579      +65     
- Misses        744      769      +25     
- Partials      208      210       +2

Impacted Files	Coverage Δ
...components/AnalyticalTable/hooks/useCellStyling.ts	`0% <0%> (ø)`
packages/main/src/components/Grid/index.tsx	`86.2% <100%> (+3.12%)`	⬆️
...src/components/Form/CurrentViewportRangeContext.ts	`100% <100%> (ø)`
...kages/main/src/components/Form/FormGroup/index.tsx	`100% <100%> (ø)`
packages/base/src/hooks/useViewportRange.ts	`87.5% <87.5%> (ø)`
packages/main/src/components/Form/index.tsx	`88.88% <88.88%> (ø)`
...ckages/main/src/components/Form/FormItem/index.tsx	`96.55% <96.55%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ed5dd6...211ac8f. Read the comment docs.

vbersch

Couldn´t find any bugs. It works as expected!
Just a couple of things:

You extracted your styles into a jss file, but you don´t actually use jss. It would be cleaner to either use jss or create the styles you need directly in your component with stable references.
You modified the Grid component and added a new event. You should be able to get the size changes with already existing apis (see comment in grid file)
Each FormItem reacts separately to viewport changes. I think it would be more efficient (and you would need less code) to only subscribe to viewport changes with the Form component and pass that information down to its children.
I think the logic to assign separate FormItems to a synthetic FormGroup can be simplified.
Could you please create lib exports for Form, FormItem and FormGroup? Have a look at main/src/libas a reference. Afterwards, could you please execute the script main/scripts/create-library-export.js?

packages/main/src/components/Form/Form.jss.ts

packages/main/src/components/Form/Form.stories.tsx

packages/main/src/components/Form/FormGroup/index.tsx

packages/main/src/components/Grid/index.tsx

packages/main/src/components/Form/hooks/useRateChanged.ts

packages/main/src/components/Form/FormItem/index.tsx

packages/main/src/components/Form/FormGroup/index.tsx

…m-component

packages/base/src/hooks/useViewportRange.ts

packages/main/src/components/Form/CurrentViewportRange.ts

packages/main/src/components/Form/FormGroup/index.tsx

packages/main/src/components/Form/index.tsx

packages/main/src/components/Grid/index.tsx

vbersch · 2019-12-12T12:05:13Z

packages/main/src/components/Form/CurrentViewportRange.ts

@@ -0,0 +1,5 @@
+import React from 'react';
+
+const CurrentRange = React.createContext(null);


We should delete this file.

packages/main/src/components/Form/Form.stories.tsx

vbersch · 2019-12-12T12:07:24Z

packages/main/src/components/Form/FormGroup/index.tsx

+      <FlexBox
+        justifyContent={FlexBoxJustifyContent.Start}
+        alignItems={FlexBoxAlignItems.End}
+        fitContainer={true}


should be a tslint error. You should omit the ={true} part.

vbersch · 2019-12-12T12:11:19Z

packages/main/src/components/Form/index.tsx

+import { TitleLevel } from '@ui5/webcomponents-react/lib/TitleLevel';
+import { styles } from './Form.jss';
+import { createUseStyles } from 'react-jss';
+import { useViewportRange } from '@ui5/webcomponents-react-base/src/hooks/useViewportRange';


you should add a lib export for useViewportRange and import from there. see e.g. lib/Device.

vbersch · 2019-12-12T12:12:15Z

packages/main/src/components/Grid/index.tsx

 import { styles } from './Grid.jss';
+import { useViewportRange } from '@ui5/webcomponents-react-base/src/hooks/useViewportRange';


see comment above. You should import from @ui5/webcomponents-react-base/lib/useViewportRange

vbersch · 2019-12-12T18:34:56Z

packages/main/src/components/Form/Form.stories.tsx

+import notes from './Form.md';
+import { FormItem } from './FormItem';
+import { FormGroup } from './FormGroup';
+import { CheckBox, Input, InputType, Option, Select } from '../..';


This is still not correct. Please import each of those components separately. e.g.
import { CheckBox } from '@ui5/webcomponents-react/lib/CheckBox';

vbersch · 2019-12-12T18:37:58Z

packages/main/src/components/Form/FormItem/index.tsx

+import { Label } from '@ui5/webcomponents-react/lib/Label';
+import { styles } from '../Form.jss';
+import { createUseStyles } from 'react-jss';
+import { CurrentRange } from '../index';


Ahhh. I know we talked about this but I now realize that we have a cyclic dependency if we do it this way.... sorry!
Form imports FormItem and FormItem imports Form. Thats not a good idea. Could we put the CurrentRangeContext in a separate file, please?

MarcusNotheis and others added 24 commits November 7, 2019 10:33

feat(polyfills): Add Safari Polyfill

ea20fa1

Merge branch 'master' into chore/safari-polyfill

3aa4f73

analytical table columns drag'n'drop implementation

3ab5b79

merged

3df54a5

Merge commit '3aa4f731cefd0c0e757fe8f2c325d5e81b062fec' into feat/ana…

ee57618

…lytical-table-dnd

unit test added

d7af885

delete unneccessary files

9f3e5d4

make dnd marker disappeared when drop occurs out of a target column; …

da3a172

…use of a standard style added; custom dnd hook added

Merge branch 'master' of ssh://github.com/SAP/ui5-webcomponents-react…

7d03370

… into feat/analytical-table-dnd

Merge branch 'master' into feat/analytical-table-dnd

f9b3726

onColumnsReordered Event added; hook renamed to correct name

c8809fe

remove incorrectly named hook file

73c3716

Merge branch 'master' into feat/analytical-table-dnd

e57e690

refactor(AnalyticalTable): update react-table and replace grid layout…

253a963

… with absolute layout BREAKING CHANGE: Update `react-table` to `7.0.0-rc.23`

WIP: Update tests

e462376

WIP: Update react-table and add resize handler

2965c2d

Update tests

7fbf85e

Update AnalyticalTable.test.tsx

17daa55

initial create

4fecbdc

more tests and touch of style

7205b0d

restyling

9e6ee24

Merge branch 'master' into refactor/AnalyticalTable

9f65904

Merge branch 'master' of ssh://github.com/SAP/ui5-webcomponents-react…

a5b2df8

… into feat/form-component

Merge commit '9f659048c527fdab126f8cb8681bd142ae4d4854' into feat/for…

789ef64

…m-component

andybessm requested a review from vbersch December 9, 2019 08:16

vbersch changed the title ~~Feat/form component~~ feat(Form): initial Implementation Dec 9, 2019

andybessm added 2 commits December 9, 2019 10:45

performance optimization; styling

d56dfc8

test snapshot updated

a217398

use hook for initialization; restyling

0134fc8

vbersch requested changes Dec 9, 2019

View reviewed changes

andybessm and others added 5 commits December 11, 2019 14:31

recreate the component according to the review results

6219f3e

recreate test snapshots

2b9c794

Merge commit '00239135eea104d4f4cf7695d0050041b6248bdd' into feat/for…

39100f1

…m-component

Merge branch 'master' into feat/form-component

028e298

Merge branch 'master' into feat/form-component

726ce06

vbersch requested changes Dec 12, 2019

View reviewed changes

MarcusNotheis and others added 2 commits December 12, 2019 08:31

WIP: Change import path of device

04079ed

fixes according to the review

99329a1

andybessm requested a review from vbersch December 12, 2019 11:49

vbersch requested changes Dec 12, 2019

View reviewed changes

fixes according to the code review

7c0d8b6

andybessm requested a review from vbersch December 12, 2019 14:05

vbersch requested changes Dec 12, 2019

View reviewed changes

andybessm added 2 commits December 12, 2019 22:39

fixes according to the code review

5ca0b25

Merge branch 'master' into feat/form-component

211ac8f

vbersch approved these changes Dec 13, 2019

View reviewed changes

vbersch merged commit c4c2848 into master Dec 13, 2019

vbersch deleted the feat/form-component branch December 13, 2019 06:38

andybessm requested a review from vbersch December 13, 2019 07:03

MarcusNotheis mentioned this pull request Dec 20, 2019

Form Component #226

Closed

		@@ -0,0 +1,5 @@
		import React from 'react';

		const CurrentRange = React.createContext(null);

		import { styles } from './Grid.jss';
		import { useViewportRange } from '@ui5/webcomponents-react-base/src/hooks/useViewportRange';

feat(Form): initial Implementation #242

feat(Form): initial Implementation #242

Uh oh!

Conversation

andybessm commented Dec 9, 2019

Pull Request Checklist

Uh oh!

netlify bot commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vbersch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

vbersch Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

netlify bot commented Dec 9, 2019 •

edited

Loading

codecov-io commented Dec 9, 2019 •

edited

Loading