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 append element #1802

Merged
merged 3 commits into from
Jan 18, 2023
Merged

Add append element #1802

merged 3 commits into from
Jan 18, 2023

Conversation

smbea
Copy link
Contributor

@smbea smbea commented Jan 11, 2023

Closes #1801
Depends on #1807

How it looks:

Uploading Screen Recording 2023-01-12 at 13.46.56.mov…

Note:
I used the same approach as for icons as we do in align and distribute. It seems to be the best solution we have for now (maybe the svg updates could be automated in the future)

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Jan 11, 2023
@smbea smbea requested review from a team, philippfromme, marstamm and nikku and removed request for a team January 11, 2023 11:50
@nikku
Copy link
Member

nikku commented Jan 11, 2023

@smbea Any reason we're not using the ... icon? As far as I am aware the icon you chose is a web modeler extension, specifically to append a new connector. I'd not want to incorporate it into the core.

If you want to I can dig up earlier discussions on "which icon to choose", where we settled on "...".

@smbea
Copy link
Contributor Author

smbea commented Jan 12, 2023

@smbea Any reason we're not using the ... icon? As far as I am aware the icon you chose is a web modeler extension, specifically to append a new connector. I'd not want to incorporate it into the core.

If you want to I can dig up earlier discussions on "which icon to choose", where we settled on "...".

I wasn't aware this is a connectors only icon (since it looks like 2 elements connected, i thought it also applied for general append). I didn't see this in the original issue, but I exchange it now for the '...'

@smbea smbea requested a review from nikku January 12, 2023 12:46
@philippfromme
Copy link
Contributor

Are we planning to add a keyboard shortcut similar to R to open the replace menu? Could really speed up modeling for expert users.

@smbea
Copy link
Contributor Author

smbea commented Jan 12, 2023

Are we planning to add a keyboard shortcut similar to R to open the replace menu? Could really speed up modeling for expert users.

I would go for a 'A' keyboard, would be really empowering for keyboard users. But I would rather add it in a separate follow up PR to keep this simpler.

@philippfromme
Copy link
Contributor

I found two quirks:

  1. You can append elements that can only be connected in the opposite direction
  2. You can append elements that cannot be connected at all

brave_brn6O8ga1Q

Are we aware of this?

@smbea
Copy link
Contributor Author

smbea commented Jan 12, 2023

I found two quirks:

  1. You can append elements that can only be connected in the opposite direction
  2. You can append elements that cannot be connected at all

Ooops. I will have another look at this

@nikku
Copy link
Member

nikku commented Jan 12, 2023

#1802 (comment)

We absolutely want that to be keyboard accessible.

In the prototype we use A (works as append with element selected, create otherwise) and N (for create new element).

@nikku
Copy link
Member

nikku commented Jan 12, 2023

@smbea Please also double-check existing bugs on the prototype. We want to make sure those don't re-open in the Camunda Modeler.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Jan 13, 2023
@smbea
Copy link
Contributor Author

smbea commented Jan 13, 2023

Some comments on the recent changes:

  • icons: you are right @nikku, I also pondered that. I started with moving the ones added here to lib/icons. I can also move the ones already present for align/distribute but I'd rather do it in a follow up PR as to not crowd this one

  • appending elements that can't be connected: Following this comment I removed the option to append elements that can't be connected. Since there isn't a canConnect utility that takes only the element type as argument, I tried to catch all these cases myself. I also see there was some discussion on boundary events and if/how they should appear on the append menu. Personally, I don't see why we would want to keep elements that can't be connected but we can have a sync on this.

  • keyboard shortcut: will follow up on this. It's already a task in the main issue.

nikku
nikku previously approved these changes Jan 13, 2023
@nikku nikku dismissed their stale review January 13, 2023 10:37

By accident.

@smbea smbea added needs review Review pending and removed in progress Currently worked on labels Jan 13, 2023
@nikku
Copy link
Member

nikku commented Jan 16, 2023

In the spirit of integrating things incrementally, this can be implemented on top, no?

Yes, of course. Let's just ensure we don't at least evaluate it at some point.

I've added it to the epic just to be sure.

@smbea
Copy link
Contributor Author

smbea commented Jan 18, 2023

In order to make it a little simpler to look at this PR, I extract the element factory fixes to #1807.

@smbea smbea marked this pull request as draft January 18, 2023 09:34
@smbea smbea marked this pull request as ready for review January 18, 2023 09:43
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jan 18, 2023
@smbea smbea requested a review from nikku January 18, 2023 09:48
@smbea
Copy link
Contributor Author

smbea commented Jan 18, 2023

Ready for review again. If you feel like this is a solid starting stage but see possible improvements, we can create follow up issues for it and them to the possible follow ups section here 🙂

@smbea smbea force-pushed the 1801-add-append branch 2 times, most recently from a05f4c8 to 30700f9 Compare January 18, 2023 10:33
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Very nice:

  • Integration with rules
  • Improved handling of icons
  • Solid test coverage
  • Really intuitive modeling

To highlight one thing I love how natural this feels to me (auto place vs. manual placement), i.e. considering the "non-interrupting boundary" placement here:

capture eoOWhe_optimized


If I had one wish then it would be to be less strict regarding when we show the menu. i.e. for link throw events:

screenshot DZKHtk

If we combine "showing it in more places" with #1621 you could argue that creating a (matching) link catch event (or creating a matching catching event for whatever throw event is selected) could be the primary action provided to a user. :)

Great work overall.

@smbea
Copy link
Contributor Author

smbea commented Jan 18, 2023

Thanks @nikku. After having the first iteration of this done, I would love to do some refining and possibly do it in the context of semantic ordering - I agree it would be awesome to have this

@nikku nikku mentioned this pull request Jan 18, 2023
19 tasks
@smbea smbea merged commit 67083cd into develop Jan 18, 2023
@smbea smbea deleted the 1801-add-append branch January 18, 2023 12:10
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add append to contextPad
3 participants