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

Embed FEEL editor in table cell #779

Merged
merged 5 commits into from
Sep 25, 2023
Merged

Embed FEEL editor in table cell #779

merged 5 commits into from
Sep 25, 2023

Conversation

barmac
Copy link
Member

@barmac barmac commented Sep 22, 2023

Closes #774

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Sep 22, 2023
@barmac barmac force-pushed the 774-feel-editor-in-table-cell branch 2 times, most recently from 6b2c40e to 9cd0e50 Compare September 22, 2023 13:03
@barmac
feat: add FEEL editor for table cells
43719ec 
Closes #774
@barmac
fix: do not close input editor on enter
b534b38
@barmac
fix: solve the replaceChild problem
a4bbf3f 
This solves a problem of `replaceChild` argument not being a child
of supposed parent. Related to changes in VDOM in reaction to focus.
@barmac
test: adjust tests
2539400
@barmac barmac force-pushed the 774-feel-editor-in-table-cell branch from 9cd0e50 to 2539400 Compare September 22, 2023 13:16
nikku
nikku approved these changes Sep 22, 2023
View reviewed changes
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.

Great stuff!

There is only a minor thing I found, which is that the code editor seems to be one pixel larger than the cell, causing a little bit of movement on focus.

capture I6EKoW_optimized

@barmac
Copy link
Member Author

barmac commented Sep 22, 2023

I cannot reproduce this o.O

@barmac
Copy link
Member Author

barmac commented Sep 22, 2023

I can reproduce it on Firefox but not on Chrome

@barmac
Copy link
Member Author

barmac commented Sep 22, 2023

Safari looks OK

@barmac
Copy link
Member Author

barmac commented Sep 22, 2023

It appears line-height: normal is different for Arial and monospace font in Firefox.
I could change all fonts in decision table to monospace, or force line-height: 1 (or other exact value).

With monospace:

Screen.Recording.2023-09-22.at.16.33.29.mov

@nikku
Copy link
Member

nikku commented Sep 22, 2023 •
edited
Loading

I would honestly not mind making the elements in the table all monospace. Makes for a much less noisy experience.

The alternative is to force the default modeling font in the FEEL editor.

My proposal is: Let's ship this (and await feedback).

@barmac barmac merged commit 4643870 into develop Sep 25, 2023
4 checks passed
@barmac barmac deleted the 774-feel-editor-in-table-cell branch September 25, 2023 07:29
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Sep 25, 2023
@nikku
Copy link
Member

nikku commented Sep 25, 2023

Let's feature the font change in the CHANGELOG as well.

@barmac
Copy link
Member Author

barmac commented Sep 25, 2023

Let's feature the font change in the CHANGELOG as well.

Done via 3edc685
Thanks for pointing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikku nikku nikku approved these changes

@philippfromme philippfromme Awaiting requested review from philippfromme

Assignees

@barmac barmac

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Embed FEEL editor in Table cells
2 participants
@barmac @nikku