Closed ChristopherSteiner closed 3 months ago
Very nice, thanks for the contribution! I'll have a deeper look later, but it looks already very good! 💪
I think it would make sense to create a custom UI for this, similar to the DeepL translator, with 2 bigger text fields for encoding and decoding. The generic search field is too small when you want to encode/decode bigger strings. What do you think? I can also help with that if you want.
Yes this sounds reasonable. I hadn't used this extension before and therefore didn't realise that you can make your own custom UIs. I will take a look at the Deepl implementation and try to adapt it to my use case as soon as I have the time. I'll let you know if I need assistance, but I think it should be pretty straight forward because the UI is more or less the same (or even easier because of the missing api key UI). Even for a TypeScript Newbie like me :)
I've removed the instant search results and implemented a new UI for this extension instead. Basically you can enter a string on either side and the string gets encoded or decoded on the other side.
It works as expected, thanks for putting your time into this project! I left some comments, apart from that it looks good to me.
By the way, are you willing to mainain this extension? This means that I'll make you the code owner of all files in the following folders:
main/Extensions/Base64Conversion
renderer/Extensions/Base64Conversion
common/Extensions/Base64Conversion
And I'll forward all issues/questions/feature requests regarding this extension to you.
And what do you think about adding some unit tests? ;)
First of all thanks for the thorough code review and explanations. I think I was able to refactor everything according to your recommendations. I was actually thinking about unit tests but then I dismissed it because the effective logic of the extensions are like two lines of code :). However I moved the encode and decode methods into there own class, I feel it's nicer like this anyway and I could test them now too.
And sure, I wouldn't mind to further maintain this extension. If I'm the owner of that code there are hopefully still the code reviews from you/another owner in the PR though?
Sorry, for more nit-picky comments, but after this it should be good to merge! Thanks again for your patience :)
If I'm the owner of that code there are hopefully still the code reviews from you/another owner in the PR though?
If I feel the need, or if you want to get help, sure I can always help out. Otherwise, I will just review the code to make sure that new code does not violate some architectural rules (which, as I said before, are not documented yet). The goal is to enable devs to maintain their extensions as independently as possible, as long as the code meets a certain standard.
Guess I should read up about the exports and imports :)
Looks good now! Thanks for your contribution.
Currently I have to encode and decode some strings with base64 on a daily basis, so I thought a simple extension for Ueli would be nice. Let me know if there are any guidelines on a preferred pattern to trigger an extensions "instant search", I was unable to find anything. Currently this extensions search gets triggered when the user entered three space seperated strings with the following format:
<some string> <b/b64/base64> <encode/decode>