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

Update FEEL toggle #245

Merged
merged 4 commits into from
May 22, 2023
Merged

Update FEEL toggle #245

merged 4 commits into from
May 22, 2023

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented May 10, 2023

Closes #240
Closes #194

Demo (with form-js): https://demo-updated-feel-toggle--camunda-form-playground.netlify.app/

image

Notes for the reviewer

  • I added an inline option to the toggle switch to be able to align non feel and feel toggle switches
  • I added some missing focus styles for the toggle switch
  • The different variants are testable via following properties
    • FEEL required: Condition => Hide if
    • FEEL optional: Image => Source
    • FEEL checkbox: Disabled
    • FEEL toggle switch: Readonly
  • This is only a visual demo, the FEEL expressions won't work for disabled and readonly, yet. Also, disabled will be a toggle switch in the future.
  • In comparison to the styles, the properties panel core uses their color palette. In the demo/form-js, I already adjusted to the themable styles.

Let me know what you think 💌

@christian-konrad
Copy link

Awesome!

The shade of blue is a little hard to differentiate from the non-active state, however, it is clearly indicated by the "=" in the field below, so i don't know if is requires a higher contrast to be easy to understand + accessible. @pinussilvestrus @RomanKostka wdyt?

Bildschirmfoto 2023-05-14 um 21 28 42

@pinussilvestrus pinussilvestrus force-pushed the 240-update-feel-toggle branch from dd889e9 to 13a0d0c Compare May 15, 2023 13:09
@RomanKostka
Copy link

Awesome!

The shade of blue is a little hard to differentiate from the non-active state, however, it is clearly indicated by the "=" in the field below, so i don't know if is requires a higher contrast to be easy to understand + accessible. @pinussilvestrus @RomanKostka wdyt?

Bildschirmfoto 2023-05-14 um 21 28 42

i reworked the icons a bit.
image

https://www.figma.com/file/CiTSN1vNETjIAy6W9jUIFp/Form-Editor?node-id=1087%3A150456&t=2UvcfDjfFfoNEIVl-1

@pinussilvestrus pinussilvestrus changed the title [WIP] Update FEEL toggle Update FEEL toggle May 15, 2023
@pinussilvestrus pinussilvestrus requested review from vsgoulart, Skaiir, RomanKostka, a team, marstamm and barmac and removed request for a team May 15, 2023 13:33
@pinussilvestrus pinussilvestrus marked this pull request as ready for review May 15, 2023 13:33
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 15, 2023
@pinussilvestrus
Copy link
Contributor Author

This is ready for review now. I added reviewers from both teams ⚔️

@RomanKostka
Copy link

first impression looks great - although the mandatory state and the standard (not enabled) state looks pretty similar.

@pinussilvestrus one thing i am wondering: is it possible to have larger tooltips which open much faster than the system tooltips?

current ones:
show up after 3s
image

optimal ones:
show up after 0.5s
image

@pinussilvestrus
Copy link
Contributor Author

one thing i am wondering: is it possible to have larger tooltips which open much faster than the system tooltips?

This is part of #202. We would have to handle this separately, but without, I think there is no other way than using the browser defaults.

@pinussilvestrus pinussilvestrus force-pushed the 240-update-feel-toggle branch from 13a0d0c to 9bfcb9f Compare May 16, 2023 10:55
@christian-konrad
Copy link

@RomanKostka on your reworked design, on gray background the toggleable fx's (non-mandatory) are not separable from those on white background.
When users start with BPMN, they will be used to the button-ish look of toggleable fx's on the BPMN properties panel, and probably they will not identify those in the form editor as toggleable. Not sure if this is really an issue.

Bildschirmfoto 2023-05-16 um 22 17 21

@christian-konrad
Copy link

christian-konrad commented May 16, 2023

@RomanKostka @pinussilvestrus since we discussed tooltips so often, I think it's time to move this epic into the PDP

@nikku
Copy link
Member

nikku commented May 17, 2023

@christian-konrad If you want to link camunda/camunda-modeler#3611 discussing tooltips, that would be great :).

Update: You already did! 🎉

@RomanKostka
Copy link

@christian-konrad

@RomanKostka on your reworked design, on gray background the toggleable fx's (non-mandatory) are not separable from those on white background.
When users start with BPMN, they will be used to the button-ish look of toggleable fx's on the BPMN properties panel, and probably they will not identify those in the form editor as toggleable. Not sure if this is really an issue.

This is indeed not perfect but from my point of view not an issue.
I explored several solutions to solve this issue but each of it came with different trade-offs.
I would propose to go with this one and check if it is a problem via tracking after some time (if noone clicks the fx-toggle it would be an indicator that it is not identified as an interactive element).

When we head into the carbonization of the modeler we anyhow need to revisit the properties panel and decide if the background-colors of modelers & form-editors properties panel should have the same color or not.

Copy link
Contributor

@vsgoulart vsgoulart left a comment

Choose a reason for hiding this comment

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

Code-wise everything looks good 👍

@pinussilvestrus
Copy link
Contributor Author

@bpmn-io/modeling-dev so far I received no feedback from you regarding this PR. Feel free to give it a try until tomorrow evening, I'd merge this one otherwise as it's blocking other topics on our side (bpmn-io/form-js#658, bpmn-io/form-js#656, and others which are WIP).

Thank you ❤️

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.

I like it a lot, UX wise. Implementation looks solid, too. Thank you! :)

@nikku nikku merged commit 0d63bb0 into main May 22, 2023
@nikku nikku deleted the 240-update-feel-toggle branch May 22, 2023 15:11
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 22, 2023
@nikku
Copy link
Member

nikku commented May 22, 2023

@pinussilvestrus You want to go ahead and release this?

@pinussilvestrus
Copy link
Contributor Author

Released via 2.1.0. Thanks a lot for the review :)

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.

Update FEEL toggle Add optional FEEL checkbox and toggle switch
5 participants