-
Notifications
You must be signed in to change notification settings - Fork 438
feat: support disabled entries in popup menu #987
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
Conversation
ad8f601
to
46f9ae7
Compare
@barmac Hey Maciej, yup I'll need some support here, this is the basic disabled entries support, we'll also need to merge an element template PR later to support getting disabled entries from the element template registry and finally an create-append-anything PR to bring it all together. |
46f9ae7
to
16365fb
Compare
Man it's a different life when CI doesn't take 30 minutes to execute, definitely less concerned about strategically sending in commits :D |
Hmm CI became brittle now. I experience timeout issues on my machine now too. |
I can reproduce it on |
Was gonna say, if we look at the commit history these errors are present even a few months back. |
How does it look with the hover state? |
Right, with hover the contrast is super low. |
There are styles to make it lighter normally, not sure why it's so dark. |
I just noticed that you also changed the hover background style which was not applied in my local build. Maybe it's good enough. |
Good contrast without hover and slightly below with, I think it's okay. If we make it any darker it becomes very unclear that this is disabled. Until they patch human vision I don't think we have a much better approach aside from completely changing the styles. |
I think it looks good enough now. I'll merge it and release it. Thanks! |
Related to camunda/camunda-modeler#5109
Proposed Changes
In the context of AO, one of the things we want to be able to do is display disabled connectors for future versions. The first step to this is this here extension of the popup menu allowing disabled entries.
PR contains:
Styling needs some advice from @YanaSegal, I can't go with this super low contrast approach but anything else seems to be too much for the light disabled look.
I've treated the header entries fundamentally different from the list entries. Header entries are only actions, so they become completely un-interactive spans if they are disabled. This already looked like the behavior we were going for if no action was supplied, so I think it makes sense, this can be changed, or again we can drop this part of the PR if this is time sensitive.
List items still need to allow for keyboard based navigation, so they can still be list selected / navigated, just not triggered via enter and mouse clicks. On that note the naming is a bit confusing here so to clarify for review context: "selected" means fake focused / highlighted, and "onSelect" is a trigger/click/enter.
Screencast.from.2025-09-18.23-49-16.webm
Can't seem to get my cursor to record, but hovering gives us a not allowed cursor.
To try it out, you can use
bpmn-js-create-append-anything
, get this PR as the dependencies (bpmn-js styles will have to be overwritten manually as diagram-js styles aren't used directly, or you can change the style loading to take it directly from djs), and then in CreateMenuProvider you can setdisabled: Math.random() < 0.5,
underlabel: label && this._translate(label),
Checklist
To ensure you provided everything we need to look at your PR:
@bpmn-io/sr
toolCloses {LINK_TO_ISSUE}
orRelated to {LINK_TO_ISSUE}