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

feat: add Feel editor #158

Merged
merged 8 commits into from
Jul 13, 2022
Merged

feat: add Feel editor #158

merged 8 commits into from
Jul 13, 2022

Conversation

marstamm
Copy link
Contributor

@marstamm marstamm commented Jun 14, 2022

This PR adds new Fields that support FEEL in InputFields and TextAreas. See the usage in the Properties Panel in this PR.

Recording 2022-07-05 at 16 51 37

test it out the integration with npx @bpmn-io/sr bpmn-io/bpmn-js-properties-panel#feel-editor -l bpmn-io/properties-panel#feel-editor

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jun 14, 2022
@marstamm marstamm force-pushed the feel-editor branch 2 times, most recently from e3bef0e to 3b91f10 Compare June 16, 2022 14:24
@marstamm marstamm marked this pull request as ready for review July 5, 2022 14:53
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jul 5, 2022
@marstamm marstamm requested review from a team, philippfromme and barmac and removed request for a team July 5, 2022 14:53
@marstamm
Copy link
Contributor Author

marstamm commented Jul 5, 2022

This is ready to be reviewed.

We should hold up merging it until after the 5.1 code freeze

@barmac
Copy link
Member

barmac commented Jul 6, 2022

I'll schedule myself to review it tomorrow.

@marstamm
Copy link
Contributor Author

marstamm commented Jul 6, 2022

Awesome! Let me know if you would like to have a sync session to go through the structure before you dive in yourself :)

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.

Two comments UI/UX wise:

  • First of all, I'm super impressed what we came up with here. I find it absolutely intuitive and simple enough (no bs)

  • I'd follow up and align the code highlight color schemes with our app. It does not look right inside the app.

@marstamm
Copy link
Contributor Author

Broke after rebase on main, I will rework it to work with the changes made in #160

@@ -0,0 +1,11 @@
import { useCallback, useRef } from 'preact/hooks';

export function useStaticCallback(callback) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add documentation for this.

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 documentation explaining what it does and where we need it:
27452e0#diff-df9adc45b7ba0c5989414adedb44be602afd3e6745cf000be415f312751041ddR3-R16

Copy link
Member

@barmac barmac left a comment

Choose a reason for hiding this comment

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

Let's merge it. I haven't noticed any problems on runtime.

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.

4 participants