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

Adjust Labels and Titles #2031

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Adjust Labels and Titles #2031

merged 4 commits into from
Dec 1, 2023

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Nov 22, 2023

@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Nov 22, 2023
@philippfromme philippfromme marked this pull request as ready for review November 22, 2023 13:55
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 22, 2023
philippfromme added a commit to bpmn-io/bpmn-js-color-picker that referenced this pull request Nov 22, 2023
@nikku
Copy link
Member

nikku commented Nov 22, 2023

Are we sure we want to do this? It is (1) a major breaking change regarding i18n and (2) does not contribute to make these menus more readable (i think).

@nikku
Copy link
Member

nikku commented Nov 22, 2023

Good thing, of course, is that it'll now be somewhat consistent across our code-base.

fake-join bot pushed a commit to bpmn-io/bpmn-js-color-picker that referenced this pull request Nov 23, 2023
@barmac
Copy link
Member

barmac commented Nov 23, 2023

If we are to merge this, we should update the translations in https://github.com/bpmn-io/bpmn-js-i18n accordingly. I don't mean that we should fix the casing there, but rather update the keys to keep the old translations.

@barmac
Copy link
Member

barmac commented Nov 23, 2023

Note that Polish translations already use sentence case: https://github.com/bpmn-io/bpmn-js-i18n/blob/main/translations/pl.js

@barmac
Copy link
Member

barmac commented Nov 23, 2023

So changes would be:

-    "Add Lane above": "Dodaj pas powyżej",
+    "Add lane above": "Dodaj pas powyżej",

@barmac
Copy link
Member

barmac commented Nov 23, 2023

BTW this PR shows that translate('Text in English') pattern, while convenient, can lead us into troubles. Any change in the labels is in fact breaking for the translations. What we could do instead is to use static labels to be replaced with English or other language variant in runtime, e.g.

// in code
translate('BOUNDARY_EVENT')

// en.json
{
  "BOUNDARY_EVENT": "Boundary event"
}

Or even more convenient:

// in code
translate('BOUNDARY_EVENT', /* dynamic values */ {}, /* English */ 'Boundary event')

// de.json
{
  "BOUNDARY_EVENT": "Grenzereignis"
}

@nikku
Copy link
Member

nikku commented Nov 23, 2023

@barmac This change will be reflected in a single commit, in the i18n repository, I hope. It will also be a breaking change for the library so integrators have to expect breaking things.

I don't think that it will be much of a hazzle for our contributors to update, if we provide guidance ➡️ Blog is the minimum to ensure they have a chance. Even better updating the translation keys (we could have an old to new English translation).

@philippfromme Please create a tracking issue to collect this and @barmac's thoughts, the repositories you touch and let's outline a roll-out strategy before merging this PR. It is a huge change (for some) and we want to handle it appropriately, with a little bit of planning.

@philippfromme philippfromme changed the title Sentence-Case Labels and Titles Adjust Labels and Titles Nov 27, 2023
@philippfromme
Copy link
Contributor Author

@nikku @barmac Please have a look at https://github.com/bpmn-io/internal-docs/issues/861 and let me know what you think. The refactoring proposed by @barmac could be a follow-up or done along with the adjustment of the labels.

@philippfromme
Copy link
Contributor Author

After we've decided not to change how translations work, this is ready to be reviewed.

"Signal start event",
"Signal start event (non-interrupting)",
"Start event",
"Sub-process",
Copy link
Member

Choose a reason for hiding this comment

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

In the docs, it's treated as a single word: https://docs.camunda.io/docs/components/modeler/bpmn/subprocesses/
While in the specification, it takes form of "Sub-Process". I think the hyphened version is correct here.

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 take the specification as the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the spec it's Sub-process.

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.

Let's clean up the commits.

Let's clearly mark this as a breaking change, so we don't forget the major version bump.

Let's turn this from chore to feat, as we believe (I believe) this is a significant improvement to the existing editor. We'd otherwise not ship it.

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 29, 2023
BREAKING CHANGE:

* translations have changed
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Dec 1, 2023
@nikku nikku merged commit c5aea1f into develop Dec 1, 2023
12 checks passed
@nikku nikku deleted the labels branch December 1, 2023 13:11
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants