-
Notifications
You must be signed in to change notification settings - Fork 139
Implement forms row layout #553
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
Conversation
2099a2b
to
68fa1d3
Compare
68fa1d3
to
852f582
Compare
852f582
to
68436d5
Compare
1ebbe93
to
85c68a0
Compare
80431e3
to
b4a0216
Compare
b4a0216
to
45325a3
Compare
45325a3
to
4709c3d
Compare
2e8b870
to
6e75c59
Compare
6e75c59
to
86d5abb
Compare
a26712d
to
beaf29f
Compare
@RomanKostka I made some adjustments, let me know what you think.
I fixed that by giving the area a bit more space (32x32px).
This is not super easy to fix as a) the drop preview is a fixed element with fixed styles, and b) the form fields are different in their styling. I would not go for implementing drop previews for each individual form element, but I adjusted the height and margins so that it shouldn't jump around now. Still, it could not be super perfect on some occasions.
This is fixed with the preview adjustments mentioned above
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small problem I found:
The cursor seems to be a bit weird, it only shows the dragging cursor if you grab on the left icon for the entire row. I think it should be any time you drag any element
packages/form-js-editor/src/features/properties-panel/groups/LayoutGroup.js
Outdated
Show resolved
Hide resolved
packages/form-js-editor/src/render/components/icons/Draggable.svg
Outdated
Show resolved
Hide resolved
packages/form-js-editor/test/spec/features/properties-panel/PropertiesPanel.spec.js
Show resolved
Hide resolved
packages/form-js-editor/src/features/properties-panel/entries/ColumnsEntry.js
Show resolved
Hide resolved
9a0575b
to
4cefaf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
i found some small misalignments:
-
the drop preview area is now off to the left side - could be because of the larger drag row indicator?
-
the drop preview border is overlapping with the select border - not important but a detail i saw
-
(future topic) we should have a look into the sizes of each component. It feels weird having elements with different sizes when they still need the same visual space in a row.
I think this makes sense, the only thing I could imagine is that it's quite weird that for most of the canvas, the dragging cursor will be active now. But semantic-wise, it makes much sense. /cc @RomanKostka |
@vsgoulart @RomanKostka I pushed some changes and adjusted most of the things you mentioned (otherwise, I left a comment). Let's hope we can merge it now 👍 (note that we will have chances for visual updates anyway with the follow-ups). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me - lets iterate on the visuals after the release.
two things we should take care of in next iterations:
the 'not allowed' curser disappears for some components.
also the space between the rows seem a bit large and could be reduced.
Screen.Recording.2023-03-28.at.12.36.52.mov
I reduced it a bit, still, we need some space to make it easy to drag between rows. For the cursor problem I opened a follow up: #576 |
Closes #560
Depends on bpmn-io/properties-panel#230
Check out the demo here.
This includes
columns
in the viewerdragging
into modulecolumns
in the editor(Potential) follow ups