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

Make FEEL Popup Links Configurable #382

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Sep 27, 2024

Proposed Changes

  • makes the links shown in the FEEL popup editor configurable
  • links can be configured by end user instead of being hard-coded in core properties panel

You'd configure it like this:

new BpmnJS({
  ...,
  propertiesPanel: {
    ...,
    feelTooltipContainer: container,
    getFeelPopupLinks: (type) => {
      if (type === 'feel') {
        return [
          {
            text: 'Learn FEEL expressions',
            href: 'https://docs.camunda.io/docs/components/modeler/feel/what-is-feel/'
          },
          {
            text: 'Try FEEL Copilot',
            href: 'https://feel-copilot.camunda.com/'
          }
        ];
      } else if (type === 'feelers') {
        return [
          {
            text: 'Learn templating',
            href: 'https://docs.camunda.io/docs/components/modeler/forms/configuration/forms-config-templating-syntax/'
          }
        ];
      }
    }
  }
});

image

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@philippfromme
Copy link
Contributor Author

cc @bastiankoerber

@nikku
Copy link
Member

nikku commented Sep 27, 2024

What about if we shorten this to "Learn FEEL", "Try co-pilot"?

Why the external launch link, when the thing clearly is a link?

@barmac
Copy link
Member

barmac commented Sep 27, 2024

If we merge this as is, this means that any @bpmn-io/properties-panel user is nudged to try out FEEL copilot, regardless if they use Camunda or not. While this may sound OK from the business perspective, this is still an MIT-licensed open source library. From that perspective, I'd rather expect the link to be plugged somehow in Camunda products instead of making it part of the upstream library. While we already have a link to the documentation in the source code, I think it's rather more stable than a link to an experimental project.
TLDR: My suggestion is to make it pluggable, and implement the link downstream.

@barmac
Copy link
Member

barmac commented Sep 27, 2024

Also note that the link in its current form does not currently contain any UTM parameters, and with the current approach we cannot fix it easily as for each adjustment a change in this library needs to be integrated downstream.

@nikku
Copy link
Member

nikku commented Sep 27, 2024

Alternative: Move the popup editor out (make it a Camunda extension), and hence be free on how we integrate Camunda docs links there. Given past discussions on the editor, that could work, too.

@philippfromme
Copy link
Contributor Author

I was expecting the pushback. I just wanted to avoid having to do a major refactoring for a single link. 🙈

@philippfromme philippfromme marked this pull request as draft September 27, 2024 12:07
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Sep 27, 2024
@lmbateman
Copy link

lmbateman commented Sep 27, 2024

What about if we shorten this to "Learn FEEL", "Try co-pilot"?

"Learn FEEL" works for me! For Copilot, I think there are a couple of things to to figure out.

  1. Generally speaking, we'll have several Copilot LLMs, and we should think about whether, in the long run, it makes sense to refer to them all (individually and collectively) as "Copilot" in most cases, or whether we want to be more specific in most cases. My vision is that we would have a unified (but not exclusive) Copilot interface where the user can type any question/request, and an LLM sitting behind that interface can route the question to the correct LLM to take care of it (BPMN Copilot, FEEL Copilot, Camunda Docs AI, etc.). In that case, I think simply using "Copilot" would make more sense, since the user doesn't have to worry about whether the LLM they're interacting with can handle their prompt.
  2. However, in this case, we're taking the user out of the Modeler to interact with a single LLM, so we may want to be specific ("FEEL Copilot") so they understand they can't ask it to generate BPMN. (Not that I expect anyone to try to use it to generate BPMN just yet, but it seems like the BPMN Copilot might make it into the product before the FEEL Copilot does.)

Why the external launch link, when the thing clearly is a link?

The design team decided to use the launch icon to indicate when a link takes you to an external site. (I personally tend to interpret it as "this link will open in a new tab", but I've just done some research and apparently not everyone shares that interpretation.)

@philippfromme philippfromme changed the title Add FEEL Copilot link to popup editor Make FEEL Popup Links Configurable Oct 17, 2024
@philippfromme
Copy link
Contributor Author

@nikku @barmac I've adjusted the pull request (and created a follow-up) to make the links configurable just like the container of the FEEL popup. My questions:

  1. Is this a good enough solution?
  2. Where would we configure the links? In bpmn-js-properties-panel, camunda-bpmn-js or the modelers?
  3. Should we keep the previous links as a default?

@nikku
Copy link
Member

nikku commented Oct 17, 2024

Should we keep the previous links as a default?

No, was a bad practice in the first place.

Where would we configure the links?

I propose in camunda-bpmn-js.

Is this a good enough solution?

It is great.

@philippfromme
Copy link
Contributor Author

Thanks @nikku I will go ahead with that solution then.

@philippfromme philippfromme force-pushed the add-feel-copilot-link branch 2 times, most recently from 8cf1bcf to b31dbb0 Compare October 17, 2024 15:19
@philippfromme philippfromme marked this pull request as ready for review October 17, 2024 15:28
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Oct 17, 2024
philippfromme added a commit to bpmn-io/bpmn-js-properties-panel that referenced this pull request Oct 17, 2024
@philippfromme
Copy link
Contributor Author

Still waiting for a review.

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 change.

@nikku
Copy link
Member

nikku commented Oct 28, 2024

Let's add a CHANGELOG entry so we don't forget and ping @bpmn-io/hto-dev @bpmn-io/modeling-dev regarding this change.

* removes existing links
* links to be added through configuration instead
@philippfromme philippfromme merged commit 97cacb6 into main Oct 29, 2024
9 checks passed
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Oct 29, 2024
@philippfromme philippfromme deleted the add-feel-copilot-link branch October 29, 2024 20:13
philippfromme added a commit to bpmn-io/bpmn-js-properties-panel that referenced this pull request Oct 31, 2024
philippfromme added a commit to bpmn-io/bpmn-js-properties-panel that referenced this pull request Oct 31, 2024
philippfromme added a commit to bpmn-io/bpmn-js-properties-panel that referenced this pull request Nov 1, 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.

4 participants