Closed Knifa closed 1 year ago
Hey @Knifa - thank you first of all for your contribution! Since you are still pushing stuff, you want me to start reviewing now, or should I wait for your go?
Hey @Knifa - thank you first of all for your contribution! Since you are still pushing stuff, you want me to start reviewing now, or should I wait for your go?
Let's hang tight for a full review the now --- think I'll try get the UX sorted before I mark it as ready then we can go from there legit!
Roger that! From what I've seen my biggest "complaint" are the translations. Apart from that everything else is looking nice. Now seeing it, I don't even hate the TS implementation - we can migrate stuff piece by piece to TS at some point.
@stylesuxx I'll take a review now! The drag/drop component could maybe do with a bit more juice in terms of hover effects, etc. but maybe splitting hairs at this point --- would be good to get it in and make it better from there (or some ideas of what it should do).
Let me know what you want from a translations perspective!
@Knifa Regarding translations, I mentioned it in a couple of places. I also see that you have a translation file in place, so if you could just move all your translations there and replace them in the template with the according call to the translation function that would be awesome. You only need to do this for EN, all other languages (as long as the file is present) will be populated by Crowdin anyway.
Could you maybe also attach some (short) sample input and expected output files? I'd like to give it a run later on, but might not have my devices available...
Thanks! I'll go ahead and put the translations placeholders in shortly.
I've just added a debug panel as a last little extra there to try diagnose some of the issues we're seeing where the process doesn't complete on some machines --- don't expect to add anything more substantial beyond this.
Also, I thought about putting together a bit of a flow diagram if it's helpful?
Test pack with inputs + example output: https://drive.google.com/file/d/1bfOlO9alk1ncx8DFv7En8ONrZQ8op50G/view?usp=sharing
Awesome, thank you. Flow diagram sounds great, the more documentation the better ;-) Feel free to add a section to the readme explaining the process. Also thanks for the test files - greatly appreciated.
I totally reworked the processing pipeline today after realising some things I hadn't before with the WebCodecs API --- think this should resolve some of the weird problems we've been seeing AND it's much more obvious what the flow is, I think. Still plan to put together some documentation.
Everywhere that has text uses the translations now, hopefully it's all OK!
@Knifa - thank you. I still see some translations that are not in the translations files - just wanted to ask if you missed to push them. It's not a big deal, I can add them myself too.
Apart from the documentation - are you happy with the current state right now, because if so, I would give it a final look over and get ready for merging.
I would really like to release a new version with all the other features and would like to include the overlay, but I don't want to put you under pressure - if you feel you need more time, then I will simply do an intermediate release and add this feature later on. Just thought it'd be cool to have this awesome feature included as one of the shiny, nice, new things :-)
@stylesuxx: Sorry, the nav and tiles totally slipped my mind! That's them added now.
And yeah, please go ahead and give me the review. I'm happy with how it's put together, generally!
Thanks for your review @stylesuxx! Think that's it all tidied up.
I've created an issue to track the remaining TODOs: https://github.com/fpv-wtf/wtfos-configurator/issues/278
This is looking good - thank you so much! I will give it another test run tomorrow and merge it if I can't find anything else to nit pick :-)
In case you are keen to keep some things in separate commits, please squash, otherwise I will squash everything into a single commit.
Nice, thanks! Happy to have it all squashed into one commit when merged. 👍
@stylesuxx I've sorted the conflicts that cropped up from the develop
force-push so this is all looking good again (there were actually none, it was just upset about the history). I've squashed it myself 💪
@Knifa - thank you for your patience and contribution - finally merged.
Time to start pulling this together! I think it works well enough that it's worth getting it merged and dealing with it from there.
Tried to keep the TS as separate as possible as suggested. All the main functionality is under
osd-overlay
in TS in it's own little bubble. The UX components are plain JS.All of the processing is done in a web worker to keep it all separate on that front.
Remaining WIP tasks, I think:
The MP4 parsing, etc. needs broken out into it's own library because it's absolutely useful for other things. Bit of a longer term goal for me.