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

Add alignment and distribution buttons to the context pad #1682

Merged

Conversation

barmac
Copy link
Member

@barmac barmac commented Jun 10, 2022

Closes #1680
Closes #1691


Requires bpmn-io/diagram-js#656

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jun 10, 2022
@nikku
Copy link
Member

nikku commented Jun 10, 2022

What UI do we aim for? Most tooks do not show all helpers right away, but instead open an "align/distribution popup", cf.:

capture 4rXti5_optimized

I'd suggest us to do the same.

@barmac
Copy link
Member Author

barmac commented Jun 10, 2022

Indeed, I also have a feeling that we should use the popup menu. This will be especially visible when we add the distribution.

@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch 2 times, most recently from 709066a to bc6e360 Compare June 10, 2022 14:41
@barmac
Copy link
Member Author

barmac commented Jun 10, 2022

So this is how it would look like directly in the context pad:

image

I think it makes sense to use a popup menu instead, as it's too cluttered right now.

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

I switched to popup menu-based implementation:
image

I am not satisfied with how it looks like. I'd rather aim for something closer to Miro's solution:

image

Note this does not include distribution buttons.

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

With distribution buttons:

image

This is a sketch and not the current implementation state.

@smbea
Copy link
Contributor

smbea commented Jun 24, 2022

I like the placement. But I think the icons feel a bit crowded, partially because being all outlines makes it harder to instantly figure out what the icon is.

In the example Nico gave, the icons appear cleaner. Also another example:

Screenshot 2022-06-24 at 11 56 47

This is just my perception. I know we are not designers and have little design resources at the moment 😅 I can try with the icons and help if needed.

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

With more space

6px 8px:

image

8px 10px:

image

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

At the moment, I am mostly re-using the icons from Camunda Modeler (the one in the context pad is mine). I am open to re-designing the icons as a next step after we agree on the layout.

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

Cross-posting a screenshot of how it's done in Miro:

image

@smbea
Copy link
Contributor

smbea commented Jun 24, 2022

@barmac makes sense. Looks nice for the first iteration, specially with more space. Nice work

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

Just added a second sketch with even more space to #1682 (comment)

@philippfromme
Copy link
Contributor

We designed these icons a long time ago. I think they're ready for an overhaul.

@nikku
Copy link
Member

nikku commented Jun 24, 2022

What we definitely want to do (mid-term) is to refresh our popup menu styles. You could follow up with it.

Reference for new pop-up menu styles should be the create/append anything popup:

image

@barmac
Copy link
Member Author

barmac commented Jun 24, 2022

What we definitely want to do (mid-term) is to refresh our popup menu styles. You could follow up with it.

It seems we do not only need to refresh styles, but also add features to the Popup Menu code. I identified grouping so far but there may be more. Or we can reimplement the menu for align&distribution, as we did in the connectors extension 😉 .

At the moment, Popup Menu can be shortly described as "give me list of header entries to display next to each other, and a list of regular entries to display one below another". This is in fact too little to implement #1682 (comment), so I will need to spend some more time on it.

@christian-konrad
Copy link

christian-konrad commented Jun 27, 2022

@barmac The latest design works for now in my opinion.
As there may be a growing set of actions for this menu(s), we can do an overhaul on all context menus or even the context pad itself in the future. Updating the icons could then also make sense (not only the alignment ones, but maybe all?) but for now, thanks for focusing on the functionality.
We should not keep 2 different context menu designs for an infinite time though.

@nikku
Copy link
Member

nikku commented Jun 27, 2022

We should not keep 2 different context menu designs for an infinite time though.

As we incorporate create/append anything into the core we'll get things aligned (again).

@barmac
Copy link
Member Author

barmac commented Jun 27, 2022

I have two ideas how we can improve the popup menu so that it can be used for this PR and possible future improvements:

  1. Minimal solution: Add grouping so that the buttons can be divided into align and distribute groups which can be seen on the UI sketch. In addition to that, set the popup menu name in the data attributes so that it can be used to style popup menu horizontally.

  2. Powerful solution: Introduce a popup-menu.render event so that an extension can opt-in to render the popup menu on its own. As a fallback, the default rendering method is used. Thus, the extension developer has more flexibility if needed but can also use old APIs.

eventBus.on('popup-menu.render', (menuId, node) => /* do stuff with node and return `true` if handled */)

For the current task of align/distribute menu, the minimal solution seems to be good enough. This is what I will implement but we can keep 2 in mind for the future.

@nikku
Copy link
Member

nikku commented Jun 27, 2022

Minimal solution: Add grouping so that the buttons can be divided into align and distribute groups which can be seen on the UI sketch. In addition to that, set the popup menu name in the data attributes so that it can be used to style popup menu horizontally.

+1 for doing that. If data-attributes are attached to the groups styling can be done naturally to achieve what you showcased.

@barmac
Copy link
Member Author

barmac commented Jun 27, 2022

New challenge: Distribution and align actions can sometimes be cancelled internally when the action does not make sense (e.g. distribution of a task with boundary event). Notice how the popup menu does not close on the capture below for alignment. It closes for distribution actions, but only because I force it:

Screen.Recording.2022-06-27.at.15.37.09.mov

The non-action buttons are similar to what Miro has, where the buttons are also displayed as disabled:

image

So I can see the following solutions:

  1. Force close with no action (cf. distribution buttons on the capture)
  2. Disable (close to alignment buttons on the capture)
  3. Perform an initial check and don't display the buttons at all if action would be cancelled internally.

I'd rather not display disabled controls as a user cannot make any use of them. I am sure we should close the popup menu once a button is clicked. I think that in the end a solution based on rules (3) is the most intuitive. I doubt a user could be surprised there is no option to distribute elements if too few are selected. Still, in the initial iteration, we could have (1), and implement rules as a next step.

@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch 4 times, most recently from 7b14df1 to 50e01e7 Compare June 27, 2022 15:35
@barmac
Copy link
Member Author

barmac commented Jun 27, 2022

I implemented option (1). Test this with:

npx @bpmn-io/sr bpmn-io/bpmn-js#1680-add-alignment-and-distribution-buttons-to-context-pad -l bpmn-io/diagram-js#group-popup-menu-items

@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch from 50e01e7 to a85103b Compare June 28, 2022 13:45
@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch from 008af35 to 62b315f Compare July 6, 2022 08:29
@barmac
Copy link
Member Author

barmac commented Jul 6, 2022

This is now ready for review.

@barmac barmac marked this pull request as ready for review July 6, 2022 08:29
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jul 6, 2022
@barmac barmac requested a review from marstamm July 6, 2022 08:29
@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch 2 times, most recently from a980d70 to 27ae402 Compare July 6, 2022 09:58
@barmac
Copy link
Member Author

barmac commented Jul 6, 2022

Screen Shot 2022-07-06 at 11 57 54
Just added some changes so that it looks OK on IE11. It won't be 1:1 as in the modern browsers but we seem to not care anymore since we use css variables 🤡

@marstamm
Copy link
Contributor

marstamm commented Jul 6, 2022

I played around with it a bit and noticed something strange. On the simple process, if I align everything center, then top, the flows get reconnected:

Recording 2022-07-06 at 14 21 41

Obviously this is not what this tool was intended for, but would be interesting to know what causes a reconnect in the first place. Is dropOnFlow triggered during alignment?

@philippfromme
Copy link
Contributor

philippfromme commented Jul 6, 2022

Is dropOnFlow triggered during alignment?

That is what's happening, yes. elements.move will trigger this behavior automatically if the new parent has a child connection that intersects: https://github.com/bpmn-io/bpmn-js/blob/develop/lib/features/modeling/behavior/DropOnFlowBehavior.js#L139

@barmac
Copy link
Member Author

barmac commented Jul 6, 2022

I guess we could prevent this, but isn't it an edge case?

@marstamm
Copy link
Contributor

marstamm commented Jul 6, 2022

I would say it is an edge case that we don't have to fix right now

@barmac
Copy link
Member Author

barmac commented Jul 6, 2022

I would say it is an edge case that we don't have to fix right now

I've created an issue for this: #1693

@philippfromme
Copy link
Contributor

Not sure why the distribution icons don't show up when selecting 3 participants. Is that expected? Couldn't find anything in the code.

image

@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

I will check this. Thanks for pointing this out.

@philippfromme
Copy link
Contributor

Otherwise, this is looking really good and very solid. Looking forward to getting that thing out of the door. 💪🏻

@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

OK it doesn't make sense. For some unknown to me reason, the legacy code disallowed distribution of participants.

@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

I will add a test case for this.

@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch from 27ae402 to 2c75c8f Compare July 7, 2022 09:25
@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

OK done @philippfromme

@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

This broke other tests 🙈

@barmac barmac force-pushed the 1680-add-alignment-and-distribution-buttons-to-context-pad branch from 2c75c8f to 8218ec3 Compare July 7, 2022 09:38
@barmac
Copy link
Member Author

barmac commented Jul 7, 2022

OK Fixed.

Copy link
Contributor

@philippfromme philippfromme left a comment

Choose a reason for hiding this comment

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

Great job! 🚀

@fake-join fake-join bot merged commit befe8b8 into develop Jul 7, 2022
@fake-join fake-join bot deleted the 1680-add-alignment-and-distribution-buttons-to-context-pad branch July 7, 2022 11:01
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants