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

Lifeline should inherit the stroke style from the actor #1140

Merged

Conversation

alexstoick
Copy link
Contributor

@alexstoick alexstoick commented Apr 6, 2023

Fixes #1137

Screenshots

image

@alexstoick alexstoick marked this pull request as ready for review April 6, 2023 20:37
@alexstoick alexstoick force-pushed the feat/lifeline-inherit-actor-style branch from 032b460 to 282bfb1 Compare April 6, 2023 22:45
Copy link
Contributor

@gavin-ts gavin-ts left a 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

d2layouts/d2sequence/sequence_diagram.go Outdated Show resolved Hide resolved
@alexstoick alexstoick force-pushed the feat/lifeline-inherit-actor-style branch from 282bfb1 to 61c6b1f Compare April 6, 2023 23:11
@gavin-ts
Copy link
Contributor

gavin-ts commented Apr 6, 2023

#557 (comment) strikes again

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

i'm thinking maybe stroke width should never affect the line. @alexstoick do you think that's something people would actually want? i suspect more often than not, they want to change the actor stroke width without line changes.

@alexstoick
Copy link
Contributor Author

alexstoick commented Apr 7, 2023

@alixander - Personally, I'm looking for a way to highlight a particular actor in a sequence diagram. Changing the lifeline color (via stroke) - is a feature I'd want.

Another completly different way of tackling this would be to allow the actor to be part of a group - and then it'd be easy to highlight as I could change the group fill.

image

This can look a bit questionable as it could start overlapping labels / arrows etc - but if we decide not to change the lifeline color this would be an option.

LMK thoughts!

@alixander
Copy link
Collaborator

@alexstoick

Changing the lifeline color (via stroke)

yeah for sure, but what about stroke width? i say we should have edges inherit a very small set of attributes from its actor.

  • stroke
  • stroke-dash

@alexstoick
Copy link
Contributor Author

@alixander - sorry; I misread your comment. I think tentatively I will say I'm OK with not inheriting the stroke-width - but I can see a scenario where it might be desirable, again for highlighting a particular life line.

@alixander
Copy link
Collaborator

yeah we can see how well color serves for that purpose, and if there's requests for stroke-width too, we can add it later

@alexstoick
Copy link
Contributor Author

alexstoick commented Apr 7, 2023

OK - I will remove the stroke-width from the PR. Anything else outstanding?

(I'm resolving the signed commit as per Gavin's comment).

@alexstoick alexstoick force-pushed the feat/lifeline-inherit-actor-style branch from 61c6b1f to 110f248 Compare April 7, 2023 18:15
@alixander
Copy link
Collaborator

nope! looks good to me

@alexstoick alexstoick force-pushed the feat/lifeline-inherit-actor-style branch 3 times, most recently from 42107df to 80ab6c2 Compare April 7, 2023 18:25
@alexstoick
Copy link
Contributor Author

alexstoick commented Apr 7, 2023

Should be all resolved now :) Thanks for your comments 🙇

Should I edit the CHANGELOG or is that something you guys manage?

@alixander
Copy link
Collaborator

please do! @alexstoick

@alexstoick alexstoick force-pushed the feat/lifeline-inherit-actor-style branch from 80ab6c2 to 2a93d21 Compare April 7, 2023 20:31
@alexstoick
Copy link
Contributor Author

@alixander - CHANGELOG updated ✅

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

🚀 🙏

@alixander alixander merged commit 189e5a0 into terrastruct:master Apr 7, 2023
@alexstoick alexstoick deleted the feat/lifeline-inherit-actor-style branch April 7, 2023 21:29
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.

Feature Request: Lifeline should inherit Actor border styles
3 participants