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.

Sign up for GitHub

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

Jump to bottom

feat: use single arrow for connection icon #1822

Merged
merged 1 commit into from
Feb 1, 2023
Merged

feat: use single arrow for connection icon #1822

merged 1 commit into from
Feb 1, 2023

Conversation

barmac
Copy link
Member

@barmac barmac commented Jan 30, 2023 •
edited by nikku
Loading

@barmac barmac requested review from a team, philippfromme and marstamm and removed request for a team January 30, 2023 10:08
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Jan 30, 2023
@barmac barmac requested a review from nikku January 30, 2023 10:08
nikku
nikku requested changes Jan 30, 2023
View reviewed changes
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.

To reduce the visual disturbance caused by this change I'd suggest to use a connection icon that is "in line" with other connections in the context pad:

image

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

barmac commented Jan 30, 2023

I can do that but it will require changes in bpmn-font.

BTW note that the default flow angle is the same as the icon I used here:

image
image

@nikku
Copy link
Member

nikku commented Jan 30, 2023

Asked for quick validation via with a bunch of other folks. If it is just me, let's keep it as you propose.

@barmac
Copy link
Member Author

barmac commented Jan 30, 2023

Hmm I think instead of bpmn-font changes it can be implemented in bpmn-js via icons module.

@barmac
Copy link
Member Author

barmac commented Jan 30, 2023 •
edited
Loading

abee05e

image

I'm not happy with this result. It looks too crispy, and even setting shape-rendering doesn't help.

@nikku
Copy link
Member

nikku commented Jan 31, 2023

@barmac We can make minor improvements to the icon to make it look more crisp:

image.

However we'll not be able to fix the issue that the new icon has a weaker visual presence then the old one. This is simply because it takes less space.

@barmac
Copy link
Member Author

barmac commented Jan 31, 2023

I experimented with text annotation:

image

@barmac
Copy link
Member Author

barmac commented Jan 31, 2023 •
edited
Loading

The wrench:
image

@nikku
Copy link
Member

nikku commented Jan 31, 2023

@barmac Let's put up a demo where we can see this end-to-end.

@barmac
Copy link
Member Author

barmac commented Jan 31, 2023

I prepared a bpmn-font update and will post a demo link soon: bpmn-io/bpmn-font#19

@barmac
Copy link
Member Author

barmac commented Jan 31, 2023

I've changed the PR to use the font from the linked pull request but now the icons appear broken. Not sure if this is due to some changes at fontello or I did sth wrong:

image

@nikku
Copy link
Member

nikku commented Jan 31, 2023 •
edited
Loading

@barmac This is due to bpmn-io/bpmn-font#19, specifically:

image

@barmac
Copy link
Member Author

barmac commented Jan 31, 2023

OK so it's fontello's fault. I will adjust the PR.

@barmac barmac force-pushed the simplify-connection branch 2 times, most recently from e585c8d to 0ec46e8 Compare January 31, 2023 13:41
@barmac
Copy link
Member Author

barmac commented Jan 31, 2023

This is how it looks like with adjustments made:

image

@marstamm
Copy link
Contributor

I love how the lines match up now, rather than just being parallel to each other ❤️
image (2)

@nikku nikku force-pushed the simplify-connection branch 2 times, most recently from 2c9b605 to 597463d Compare January 31, 2023 17:21
@nikku
Copy link
Member

nikku commented Jan 31, 2023

I still love the old connect icon in the way it fills a square place, but hey, let's embrace the future.

Maybe a future follow-up could be to make the connection line width the same width like the annotation line (text annotation).

@philippfromme
Copy link
Contributor

I think it looks just fine.

@nikku nikku mentioned this pull request Feb 1, 2023
Merged
@nikku
Copy link
Member

nikku commented Feb 1, 2023 •
edited
Loading

@philippfromme Consider #1825 as an alternative. Which one works better?

This PR #1825
image image
image image

@nikku nikku self-requested a review February 1, 2023 08:41
@philippfromme
Copy link
Contributor

@nikku The second one works much better.

@marstamm
Copy link
Contributor

marstamm commented Feb 1, 2023

+1 for #1825

@nikku
Copy link
Member

nikku commented Feb 1, 2023

Incorporated the final icons via 2e6b976:

image

@nikku nikku merged commit 85aa6a6 into develop Feb 1, 2023
@nikku nikku deleted the simplify-connection branch February 1, 2023 10:57
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Feb 1, 2023
@nikku nikku mentioned this pull request Feb 8, 2023
Closed
41 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

@philippfromme philippfromme Awaiting requested review from philippfromme philippfromme was automatically assigned from bpmn-io/modeling-dev

@marstamm marstamm Awaiting requested review from marstamm marstamm was automatically assigned from bpmn-io/modeling-dev

Assignees

@barmac barmac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@barmac @nikku @marstamm @philippfromme