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

Resize form fields #593

Merged
merged 9 commits into from
Apr 20, 2023
Merged

Resize form fields #593

merged 9 commits into from
Apr 20, 2023

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Apr 3, 2023

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Apr 3, 2023
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 3, 2023 13:43 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 3, 2023 13:43 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 3, 2023 13:46 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 11, 2023 09:53 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 11, 2023 12:53 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 11, 2023 13:55 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 13, 2023 11:27 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 13, 2023 11:32 Destroyed
@pinussilvestrus pinussilvestrus marked this pull request as ready for review April 13, 2023 11:52
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 13, 2023
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 13, 2023 11:52 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 13, 2023 11:52 Destroyed
Copy link
Contributor

@christian-konrad christian-konrad left a comment

Choose a reason for hiding this comment

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

Found some minor issues (as mentioned in Slack), but they aren't related to the resize handles.
Really like the smooth handling of the resize handles!

*
* @return {Function} drag start callback function
*/
export function createDragger(fn, dragPreview) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference: I had a look at some existing resizable component solutions (e.g., https://github.com/react-grid-layout/react-resizable, https://www.npmjs.com/package/preact-moveable), but none of them gave enough flexibility to achieve what we wanted; especially due to the fact we resize in columns, not pixels.

I decided to re-use proven utilities from bpmn-io to implement the behaviors.

@christian-konrad
Copy link
Contributor

@pinussilvestrus for the handle height, can't we add a max-height of like calc(100% - margin) of the container size?

@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 14, 2023 09:31 Destroyed
@pinussilvestrus
Copy link
Contributor Author

@pinussilvestrus for the handle height, can't we add a max-height of like calc(100% - margin) of the container size?

Oh yeah, damn right 😅 I thought it too complicated. Just pushed it, demo should be up in a few minutes 👍

Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

@pinussilvestrus 2 things I found

  1. Maybe we should disable the handle if an element is the first column of the row?
2023-04-14.15-15-02.mp4
  1. Should we maybe prevent user from breaking into a new row?
2023-04-14.15-15-53.mp4

@pinussilvestrus
Copy link
Contributor Author

Maybe we should disable the handle if an element is the first column of the row?

Not sure about this one, it could be confusing for users if that's missing for just this case. wdyt @RomanKostka?

Should we maybe prevent user from breaking into a new row?

That should not happen, but I also see this behavior outside of resizing, also cf. to the findings Christian made with auto columns. I didn't see it with resizing yet. Can you maybe share your setup? So the columns of each form field and maybe whether you are on a smaller screen?

Copy link
Contributor

@Skaiir Skaiir left a comment

Choose a reason for hiding this comment

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

Looks great man 👍, just a few small behavior things that might be worth looking at, but otherwise what a contribution.

@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 17, 2023 09:43 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 17, 2023 09:47 Destroyed
@pinussilvestrus
Copy link
Contributor Author

To consider as an improvement on top: #604. It depends on whether we introduce single step columns or not.

Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

Implementation looks good (just made some minor comments), let's just wait to hear Roman's comment about the points I raised earlier

@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 18, 2023 11:04 Destroyed
@github-actions github-actions bot temporarily deployed to demo-566-form-field-resize April 18, 2023 12:46 Destroyed
@RomanKostka
Copy link

Not sure about this one, it could be confusing for users if that's missing for just this case. wdyt @RomanKostka?

I would propose to leave it as it is and keep the handles. Feels natural at least for me - this is something we can investigate further when we talk to some users

One other thing to mention to keep track of it:
we have to find a solution that something like this (4 components with col-span 2 in a small window):
image
is not happening too often. One solution might be to increase the threshold of the breakpoint.

@pinussilvestrus
Copy link
Contributor Author

One other thing to mention to keep track of it:
we have to find a solution that something like this

It's a thing I spent an eye for quite a while now. The thing is that Carbon calculates the breakpoints on the screen basis, not container basis, as container based media queries (CSS) are not well supported across browsers. So it doesn't matter whether you are in a single form context, or have setups like the playground were a lot is going on. Increasing the breakpoint might solve it for quite some cases, but probably not every.

Definitely a thing we should keep tracking, and act on once we need 👍

@pinussilvestrus pinussilvestrus merged commit 5b9cb54 into develop Apr 20, 2023
@pinussilvestrus pinussilvestrus deleted the 566-form-field-resize branch April 20, 2023 06:20
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add odd options for columns Redesign of delete field icon Implement form field resizing on the grid
5 participants