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

fix(assets): display bpmn-element images #4620

Merged
merged 4 commits into from
Nov 20, 2024
Merged

fix(assets): display bpmn-element images #4620

merged 4 commits into from
Nov 20, 2024

Conversation

timstawowski
Copy link
Contributor

@timstawowski timstawowski commented Nov 19, 2024 •
edited
Loading

Description

Images of bpmn-elements couldn't be displayed because of sized image tags.

Before
missing-bpmn-elements

After
with-bpmn-elements

When should this change go live?

  • This is a bug fix, security concern, or something that needs urgent release support.
  • This is already available but undocumented and should be released within a week.
  • This on a specific schedule and the assignee will coordinate a release with the DevEx team. (apply hold label or convert to draft PR)
  • This is part of a scheduled alpha or minor. (apply alpha or minor label)
  • There is no urgency with this change and can be released at any time.

PR Checklist

  • My changes are for an already released minor and are in /versioned_docs directory.
  • My changes are for the next minor and are in /docs directory (aka /next/).

@christinaausley christinaausley added kind/bug Issues related with bugs in the documentation component:modeler Issues related with Modeler project deploy Stand up a temporary docs site with this PR labels Nov 19, 2024
@christinaausley christinaausley requested a review from a team November 19, 2024 20:28
@akeller
Copy link
Member

akeller commented Nov 19, 2024

I don't think our deploy label workflow will work for externals. We'll need to review this one by running locally @christinaausley, but it was a good idea to try to add it!

akeller
akeller reviewed Nov 19, 2024
View reviewed changes
docs/components/best-practices/development/understanding-transaction-handling-c7.md Outdated
@@ -92,27 +92,27 @@ Aside a general strategy to mark service tasks as being save points you will oft

- _User tasks_ <img src="/img/bpmn-elements/task-user.svg" width="40"/>: This savepoint allows users to complete their tasks without waiting for expensive subsequent steps and without seeing an unexpected rollback of their user transaction to the waitstate before the user task. Sometimes, e.g. when validating user input by means of a subsequent step, you want exactly that: rolling back the user transaction to the user task waitstate. In that case you might want to introduce a savepoint right after the validation step.
Copy link
Member

Choose a reason for hiding this comment

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

image

This one is also not visible on the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the missing ones.

akeller
akeller requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@akeller akeller left a comment

Choose a reason for hiding this comment

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

First, I want to thank you for your attention to detail and tenacity in trying to resolve this!

Second, while removing the width does make the images visible, the experience is still not great.

image

We are incremental in our delivery and shipping, so I would accept this PR if all the images are appearing, even if the experience is not so great. We can adjust that in a separate PR.

Your PR also showed me some other issues with how our BPMN.io images/diagrams are rendering. They are visible but cut off. I will create a backlog item for this.

@akeller
Copy link
Member

akeller commented Nov 19, 2024

First, I want to thank you for your attention to detail and tenacity in trying to resolve this!

Second, while removing the width does make the images visible, the experience is still not great.

image We are incremental in our delivery and shipping, so I would accept this PR if all the images are appearing, even if the experience is not so great. We can adjust that in a separate PR.

Your PR also showed me some other issues with how our BPMN.io images/diagrams are rendering. They are visible but cut off. I will create a backlog item for this.

I created #4628 and #4629 after reviewing your PR here! 😄 Teamwork!

@mesellings mesellings mentioned this pull request Nov 20, 2024
Closed
@akeller akeller removed the deploy Stand up a temporary docs site with this PR label Nov 20, 2024
akeller
akeller approved these changes Nov 20, 2024
View reviewed changes
Copy link
Member

@akeller akeller left a comment

Choose a reason for hiding this comment

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

Thank you!

@akeller akeller merged commit a4e0b89 into camunda:main Nov 20, 2024
6 of 8 checks passed
@pepopowitz pepopowitz mentioned this pull request Nov 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akeller akeller akeller approved these changes

Assignees

@timstawowski timstawowski

Labels
component:modeler Issues related with Modeler project kind/bug Issues related with bugs in the documentation
Projects
Developer Experience
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@timstawowski @akeller @christinaausley