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

chore: use element as key for entries #176

Merged
merged 1 commit into from
Aug 18, 2022
Merged

Conversation

marstamm
Copy link
Contributor

related to bpmn-io/bpmn-js-properties-panel#712

test it out using

npx @bpmn-io/sr bpmn-io/bpmn-js-properties-panel -l bpmn-io/properties-panel#use-element-as-key

@marstamm marstamm requested a review from a team August 17, 2022 14:12
@marstamm marstamm self-assigned this Aug 17, 2022
@marstamm marstamm requested review from philippfromme and pinussilvestrus and removed request for a team August 17, 2022 14:12
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Aug 17, 2022
@pinussilvestrus
Copy link
Contributor

Just to make sure, does this still work with the Linter (showing the errors in the Properties panel, although the inputs are not visible)?

Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

I integration tested this one together with Linting in the Camunda Desktop Modeler, and everything seems to work as expected. 👍

I like the approach to have the entries unique per selected element. If we ever come up with cross-element components that share its state, we can reconsider this. This kind of entry would have to be a custom one currently anyway.

I'd be up for trying this out but wait for another ✅ as it is a pretty core change.

@nikku
Copy link
Member

nikku commented Aug 18, 2022

@pinussilvestrus Do you approve this? If so, please ✔️ 🙂

Copy link
Contributor

@pinussilvestrus pinussilvestrus left a comment

Choose a reason for hiding this comment

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

Go for it 🤞

@philippfromme philippfromme merged commit 6075388 into main Aug 18, 2022
@philippfromme philippfromme deleted the use-element-as-key branch August 18, 2022 17:52
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants