Closed ulli-hoeness closed 3 months ago
@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 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.
@ulli-hoeness Could you follow-up according to my latest feedback (https://github.com/bpmn-io/properties-panel/pull/362#issuecomment-2139102744)? We don't do merges in this library, as we emphasize a clean history.
@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.
Thank you! Will correct the placeholder behaviour today.
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.
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
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
Thank you for the follow-up! Looks good to me now, I'll go ahead and release the changes. Thank you for your contribution 🏆
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.