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

BKM as literal expression #852

Merged
merged 31 commits into from
May 6, 2024
Merged

BKM as literal expression #852

merged 31 commits into from
May 6, 2024

Conversation

barmac
Copy link
Member

@barmac barmac commented Apr 19, 2024

Screen.Recording.2024-04-25.at.17.17.18.mov

Closes #826

Test via npx @bpmn-io/sr bpmn-io/dmn-js#826-bkm-as-literal-expression

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Apr 19, 2024
@barmac
Copy link
Member Author

barmac commented Apr 19, 2024

TODO:

  • Rebase on top of main
  • Make sure function kind doesn't change its size unnecessarily
  • Include parameters in the suggestions
  • ...

@barmac barmac force-pushed the 826-bkm-as-literal-expression branch 2 times, most recently from 48c9fd6 to 2a049ac Compare April 22, 2024 13:49
@barmac
Copy link
Member Author

barmac commented Apr 22, 2024

Include parameters in the suggestions

Extracted to #853 as it needs to be solved in https://github.com/bpmn-io/dmn-variable-resolver

@barmac barmac force-pushed the 826-bkm-as-literal-expression branch from 2a049ac to 29b600d Compare April 23, 2024 08:14
@barmac
Copy link
Member Author

barmac commented Apr 23, 2024

This should work but doesn't:
npx @bpmn-io/sr bpmn-io/dmn-js#826-bkm-as-literal-expression

I will update the demo website.

@barmac
Copy link
Member Author

barmac commented Apr 24, 2024

There was no need to update the website as the one-liner above works just fine.

@barmac barmac force-pushed the 826-bkm-as-literal-expression branch from 38e707e to 5811318 Compare April 24, 2024 09:29
@barmac
Copy link
Member Author

barmac commented Apr 29, 2024

It looks like we are suffering from npm/cli#4828 on the CI

@barmac barmac force-pushed the 826-bkm-as-literal-expression branch from 3b4699e to e499c25 Compare April 29, 2024 10:08
@barmac barmac marked this pull request as ready for review April 29, 2024 10:10
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 29, 2024
@barmac barmac requested review from a team, philippfromme and marstamm and removed request for a team April 29, 2024 10:10
@barmac barmac requested a review from nikku April 29, 2024 10:13
@barmac
Copy link
Member Author

barmac commented Apr 29, 2024

Given we add an input-like border around the name, is it sufficient?

I'm sorry, I misunderstood. I meant that I thought it was enough to have the border around the name when it's in edit mode, since the user can edit the BKM name elsewhere. I'm not sure about the border being visible all the time; without a visible label, the box feels a bit non-standard to me. If we could make the name consistent with the other header items and show the edit button on hover, I think that might be best. @crobbins215, any thoughts? (Apologies again for the misunderstanding.)

I'd say let's keep this in mind for the follow-up improvements, so that we can learn from user's feedback.

@barmac
Copy link
Member Author

barmac commented Apr 29, 2024

@philippfromme @nikku @marstamm Please have a look at this. There are some follow-up issues linked in the product hub, but I'd like to work on them separately from this PR.

@barmac barmac mentioned this pull request Apr 30, 2024
2 tasks
@barmac
Copy link
Member Author

barmac commented Apr 30, 2024

TODO: Make inputs commit on blur, and have a separate command stack (browser native) instead of diagram-js command stack.

Copy link
Contributor

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

Found some smaller things that we can address, but nothing major. As discussed, I opened #858 as it is not a regression

@barmac
Copy link
Member Author

barmac commented Apr 30, 2024

TODO: Make inputs commit on blur, and have a separate command stack (browser native) instead of diagram-js command stack.

I will extract this in a separate issue. This is broken already in dmn-js-literal-expression, and it's a bigger issue than I thought because we need to change multiple components to make it work correctly.

@barmac
Copy link
Member Author

barmac commented Apr 30, 2024

I created #859 as a follow-up.

@barmac barmac requested a review from marstamm April 30, 2024 13:19
Copy link
Contributor

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

With the Follow-ups we discussed, I think we are good to merge. Great Job 🚀

@barmac barmac merged commit 7764a0a into develop May 6, 2024
8 checks passed
@barmac barmac deleted the 826-bkm-as-literal-expression branch May 6, 2024 10:34
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 6, 2024
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.

Implement BKM as literal expression
3 participants