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

Enhance ListGroup and Collapsible components to support translation and add tests for translation #362

Merged

Conversation

ulli-hoeness
Copy link
Contributor

Closes #358

@barmac Sorry for opening a new pull request. I was signed in with the wrong user and was unable to sign the CLA.

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.

@ulli-hoeness Please update on latest main, there is a couple of changes that your change conflict with (removed sorting).

@ulli-hoeness
Copy link
Contributor Author

@ulli-hoeness Please update on latest main, there is a couple of changes that your change conflict with (removed sorting).

I merged main into my branch. I see the merge locally but i do not see it in the commits in this pull request although i have pushed everything. Working with github is new to me so I apologize if the changes are not as requested. Please have a look and if my branch is still behind, I will fix it.

@ulli-hoeness ulli-hoeness requested a review from nikku May 28, 2024 14:37
@nikku
Copy link
Member

nikku commented May 30, 2024

@ulli-hoeness You'd want to rebase your branch on top of main for a clear history:

git checkout main
git pull
git checkout your-feature-branch
git rebase main

You'll be able to see merge conflicts (if they appear), fix them and have a clean history.

@nikku
Copy link
Member

nikku commented Jun 7, 2024

@ulli-hoeness Could you follow-up according to my latest feedback (#362 (comment))? We don't do merges in this library, as we emphasize a clean history.

@ulli-hoeness
Copy link
Contributor Author

@ulli-hoeness You'd want to rebase your branch on top of main for a clear history:

git checkout main
git pull
git checkout your-feature-branch
git rebase main

You'll be able to see merge conflicts (if they appear), fix them and have a clean history.

@nikku Thank you! I apologize for the delayed response. Unfortunately I haven't had the time to deal with this yet. Will rebase main onto my branch within the next few days. Will update you when it is ready for review.

@nikku nikku added the needs review Review pending label Jun 11, 2024 — with bpmn-io-tasks
@nikku nikku requested a review from marstamm June 13, 2024 08:24
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jun 13, 2024
@marstamm marstamm force-pushed the feature/add_translation_in_listgroup branch 2 times, most recently from 93ac275 to 92ecaa0 Compare June 13, 2024 08:45
@marstamm marstamm force-pushed the feature/add_translation_in_listgroup branch from 92ecaa0 to d973379 Compare June 13, 2024 08:48
Copy link
Contributor

@marstamm marstamm left a comment

Choose a reason for hiding this comment

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

I have rebased the branch for you. Please check the test failures:

The current implementation breaks if you use placeholders, as they are not replaced by the fallback functions, e.g.

translate(`List contains {numOfItems}`, { numOfItems: 15 })
// Results in "List contains {numOfItems}"

We want to ensure it still works without providing a translation provider.

@nikku
Copy link
Member

nikku commented Jun 13, 2024

Cf. base translate implementation.

@ulli-hoeness
Copy link
Contributor Author

Thank you! Will correct the placeholder behaviour today.

…te function was passed as a prop. If not, the strings are returned as they are. A fallback function of (string) => string was not viable because of replacement values in one of the strings.
@ulli-hoeness
Copy link
Contributor Author

ulli-hoeness commented Jun 14, 2024

I have rebased the branch for you. Please check the test failures:

The current implementation breaks if you use placeholders, as they are not replaced by the fallback functions, e.g.

translate(`List contains {numOfItems}`, { numOfItems: 15 })
// Results in "List contains {numOfItems}"

We want to ensure it still works without providing a translation provider.

@marstamm Thank you for the Rebase and pointing out the bug in the code. Please have a look if the conditional use of the translate function in the newest commit is okay. You pointed out correctly that the previously used fallback function of (string) => string does not work as desired for string which use raplace values. Since no other component contained string which use replace values, I did leave the fallvack function for translate as it was. Only in ListGroup we check if translate exists.

The tests still show 4 failed tests for FeelField, FeelEditor and Templating. I am not aware of having changed anything concerning these components though.

@marstamm
Copy link
Contributor

Thank you for the follow-up! While this works for the current state, we introduce 2 patterns to deal with missing translation provider. Either we have a fallback function or use the ternary. We should aim to have a consistent pattern for translations.

Having a fallback translation provider as a utility function (cf. Nikku's comment) would be a more elegant solution. It would ensure that we can use translate everywhere and is a pattern we use in other libraries as well.

You can create a shared utility in https://github.com/bpmn-io/properties-panel/tree/main/src/components/util and use it as a fallback in both locations

translate fallback function was copied from bpmn-io/diagram-js
@ulli-hoeness
Copy link
Contributor Author

As requested, I have used the linked code for the translate fallback and have placed it in src/components/util. This fallback is now used in case no translation module is present

@marstamm
Copy link
Contributor

Thank you for the follow-up! Looks good to me now, I'll go ahead and release the changes. Thank you for your contribution 🏆

@marstamm marstamm merged commit ee3e7cc into bpmn-io:main Jun 14, 2024
7 checks passed
@marstamm
Copy link
Contributor

Released as v3.21.0

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.

Add missing translation capabilities to ListGroup
3 participants