search

pangeachat / client

Learn a language while texting your friends
https://krille-chan.github.io/fluffychat/
GNU Affero General Public License v3.0
1 stars 2 forks source link

Improve interactive translator [WIP] #319

Closed casualWaist closed 2 weeks ago

casualWaist commented 1 month ago

Added auto play and animated transitions as well as a few fixes. Will revisit animation and auto play pop up doesn't show on first submission.

wcjord commented 1 month ago

Hey @casualWaist! Checking out the branch, code and app. The set of changes are definitely improving the fluidity of the IT experience.

I like the addition of the autoplay feature. It's nice! Let's set this to false by default so the learner sees the dialog in the first set of interactions before setting it to auto. I edited the text to trim a bit.

On the code side, to keep things cleaner and more modularized, I'd rather the toggle widget handle the logic of saving to the user's profile directly and keep this out of the broader igc controller. This can happen directly when the user clicks the toggle. I don't really see a need to wait until they've clicked the ignore/accept button.

Animation-wise. The opening of the IT bar is a bit jumpy and I'm hoping you can smooth it out a bit. There is another feature that expands the text bar to include a message about the app connectivity. To see it, a) run the app b) open the web developer console c) go to the Network tab d) change from "no throttling" to "offline" and e) watch the input bar for ~30 seconds. The input bar expands to accommodate a message and is quite smooth. How can we make the input bar to expand and accommodate the IT in a manner that is that smooth?

casualWaist commented 1 month ago

I was actually having trouble figuring out how to set the value to false when it was initialized. I’ll jump back into that at some point today.

That’s cool. I’ll make those changes. I was on a bit of a journey with figuring out how to get this to work and when it finally was I didn’t really take time to rethink it, but that seems like an easy change to make.

I’ve actually seen that network error expansion a bunch as my wifi tends to drop out at random times. Lol. I was having the same problems with the sputtery animation and learned online that running the app in profile mode provides more accurate representations of how the app will perform animations real world. This made sense to me as React has similar issues with dev mode. For me, putting the app into profile mode totally smoothed out the animations. Does this work for you? My research online seemed to suggest that this was okay but if that’s not your experience let me know and we figure out something else.

On Jun 14, 2024, at 9:05 AM, wcjord @.***> wrote:

Hey @casualWaist https://github.com/casualWaist! Checking out the branch, code and app. The set of changes are definitely improving the fluidity of the IT experience.

I like the addition of the autoplay feature. It's nice! Let's set this to false by default so the learner sees the dialog in the first set of interactions before setting it to auto. I edited the text to trim a bit.

On the code side, to keep things cleaner and more modularized, I'd rather the toggle widget handle the logic of saving to the user's profile directly and keep this out of the broader igc controller. This can happen directly when the user clicks the toggle. I don't really see a need to wait until they've clicked the ignore/accept button.

Animation-wise. The opening of the IT bar is a bit jumpy and I'm hoping you can smooth it out a bit. There is another feature that expands the text bar to include a message about the app connectivity. To see it, a) run the app b) open the web developer console c) go to the Network tab d) change from "no throttling" to "offline" and e) watch the input bar for ~30 seconds. The input bar expands to accommodate a message and is quite smooth. How can we make the input bar to expand and accommodate the IT in a manner that is that smooth?

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2168001994, or unsubscribe https://github.com/notifications/unsubscribe-auth/A4QVILTODWX5EGTPKKPF6ODZHLTB3AVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGAYDCOJZGQ. You are receiving this because you were mentioned.

wcjord commented 1 month ago

👍

👍

My experience is that production always shows more issues than dev and if an issue is apparent in dev, it's definitely there in at least some situations on production. I'd be surprised if this particular issue is different. I know that app connectivity message displays well in dev mode as well. The widget is ConnectionStatusHeader and it's in a column in chat_view. I think moving the ITBar to this column could improve the animation.

On Fri, 14 Jun 2024 at 09:24, Matt @.***> wrote:

I was actually having trouble figuring out how to set the value to false when it was initialized. I’ll jump back into that at some point today.

That’s cool. I’ll make those changes. I was on a bit of a journey with figuring out how to get this to work and when it finally was I didn’t really take time to rethink it, but that seems like an easy change to make.

I’ve actually seen that network error expansion a bunch as my wifi tends to drop out at random times. Lol. I was having the same problems with the sputtery animation and learned online that running the app in profile mode provides more accurate representations of how the app will perform animations real world. This made sense to me as React has similar issues with dev mode. For me, putting the app into profile mode totally smoothed out the animations. Does this work for you? My research online seemed to suggest that this was okay but if that’s not your experience let me know and we figure out something else.

On Jun 14, 2024, at 9:05 AM, wcjord @.***> wrote:

Hey @casualWaist https://github.com/casualWaist! Checking out the branch, code and app. The set of changes are definitely improving the fluidity of the IT experience.

I like the addition of the autoplay feature. It's nice! Let's set this to false by default so the learner sees the dialog in the first set of interactions before setting it to auto. I edited the text to trim a bit.

On the code side, to keep things cleaner and more modularized, I'd rather the toggle widget handle the logic of saving to the user's profile directly and keep this out of the broader igc controller. This can happen directly when the user clicks the toggle. I don't really see a need to wait until they've clicked the ignore/accept button.

Animation-wise. The opening of the IT bar is a bit jumpy and I'm hoping you can smooth it out a bit. There is another feature that expands the text bar to include a message about the app connectivity. To see it, a) run the app b) open the web developer console c) go to the Network tab d) change from "no throttling" to "offline" and e) watch the input bar for ~30 seconds. The input bar expands to accommodate a message and is quite smooth. How can we make the input bar to expand and accommodate the IT in a manner that is that smooth?

— Reply to this email directly, view it on GitHub < https://github.com/pangeachat/client/pull/319#issuecomment-2168001994>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/A4QVILTODWX5EGTPKKPF6ODZHLTB3AVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGAYDCOJZGQ>.

You are receiving this because you were mentioned.

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2168040854, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYPKFOZTSSKI4AU2C4JSI3ZHLVKXAVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGA2DAOBVGQ . You are receiving this because you commented.Message ID: @.***>

wcjord commented 3 weeks ago

@casualWaist Hey did you see the comment about moving the IT bar to where the ConnectionStatusHeader is located as a way to make the animation more fluid? I'd like you to experiment with this.

I love the new animations! Now, I want to see them in more places! Can you add the bot into the ITBar? Once upon a time, it was in the upper left corner of that widget. With the new animations, it would be great to have it there again.

Last, the expressions are all really fun. I'd love to see surprised used somewhere. Maybe when the user goes with a nongold option? That is, selects it and then actually uses it.

casualWaist commented 3 weeks ago

Oh man, my bad Will. I got so into the other stuff I forgot to get back to it. I’ll give it a try today and let you know how it works out.

On Jun 18, 2024, at 10:59 AM, wcjord @.***> wrote:

@casualWaist https://github.com/casualWaist Hey did you see the comment about moving the IT bar to where the ConnectionStatusHeader is located as a way to make the animation more fluid? I'd like you to experiment with this.

I love the new animations! Now, I want to see them in more places! Can you add the bot into the ITBar? Once upon a time, it was in the upper left corner of that widget. With the new animations, it would be great to have it there again.

Last, the expressions are all really fun. I'd love to see surprised used somewhere. Maybe when the user goes with a nongold option? That is, selects it and then actually uses it.

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2176319570, or unsubscribe https://github.com/notifications/unsubscribe-auth/A4QVILR7FINAB4ORE6SGJILZIBDLPAVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZWGMYTSNJXGA. You are receiving this because you were mentioned.

casualWaist commented 3 weeks ago

I didn't add anything to the updateEditableClassField because this seemed like a mistake to me, but everything should be as asked. Let me know if there is anything else I can fix.

wcjord commented 3 weeks ago

@ggurdin Hey give this a final look please, we'll ask @casualWaist to resolve the conflicts and get it merged into main!

ggurdin commented 3 weeks ago

@wcjord @casualWaist Looks good! One weird thing - if IT autoplay is disabled on the class level, the user can still toggle their user settings in the IT start popup. It's confusing to toggle that, start the next round of IT, and have that toggle turned off again (because it's disabled on the class level).

wcjord commented 3 weeks ago

Hm that's probably a good reason to not have it in those settings. It isn't something that a teacher really needs control over.

On Fri, Jun 21, 2024 at 1:45 PM ggurdin @.***> wrote:

@wcjord https://github.com/wcjord @casualWaist https://github.com/casualWaist Looks good! One weird thing - if IT autoplay is disabled on the class level, the user can still toggle their user settings in the IT start popup. It's confusing to toggle that, start the next round of IT, and have that toggle turned off again (because it's disabled on the class level).

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2183177584, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYPKFPMUIE54WH2HTE7DQDZIRRB7AVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBTGE3TONJYGQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Will Jordan-Cooley CEO and Founder Pangea Chat https://www.pangeachat.com

casualWaist commented 2 weeks ago

Hey, so is the consensus to undo the changes in the last push, save for the localization fix?

wcjord commented 2 weeks ago

@ggurdin is there some other solution that better organizes things but doesn't put them under the purview of the teacher to edit? Let's discuss tomorrow when I'm back in the office.

On Mon, Jun 24, 2024 at 8:55 AM Matt @.***> wrote:

Hey, so is the consensus to undo the changes in the last push, save for the localization fix?

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2186516181, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYPKFPRRZEP4KO2BUGDRD3ZJAJL7AVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBWGUYTMMJYGE . You are receiving this because you were mentioned.Message ID: @.***>

-- Will Jordan-Cooley CEO and Founder Pangea Chat https://www.pangeachat.com

ggurdin commented 2 weeks ago

There's a way to have a setting that is in the user profile and not in the class settings - @casualWaist it might be helpful to look at how the autoPlayMessages setting is handled in the enum MatrixProfile. It's not in the ToolSettings enum, which is used to populate class settings, but it is used to set/get user settings.

casualWaist commented 2 weeks ago

Okay, I see now. I didn't realize the Room rules were pulling from ToolSettings. I had tried modeling this setting after autoPlayMessages before but I ran into a wall. I don't remember what happened but will try it like this again.

wcjord commented 2 weeks ago

Hey Matt, just hold off until Gabby or I gets back to you with a clear solution. We've got a couple competing goals and need to decide which to prioritize slash find a third solution.

On Mon, Jun 24, 2024 at 2:52 PM Matt @.***> wrote:

Okay, I see now. I didn't realize the Room rules were pulling from ToolSettings. I had tried modeling this setting after autoPlayMessages before but I ran into a wall. I don't remember what happened but will try it like this again.

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2187200834, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYPKFKRSDRUKTRNE4SWTJ3ZJBTFBAVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXGIYDAOBTGQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Will Jordan-Cooley CEO and Founder Pangea Chat https://www.pangeachat.com

casualWaist commented 2 weeks ago

👍

wcjord commented 2 weeks ago

@casualWaist Hey we talked about it and we'd like to do it the same as autoPlayMessages which you can search in the project and implement just like that. Let us know if you have any further issue with it.

casualWaist commented 2 weeks ago

okay, I should be able to get to it this afternoon

wcjord commented 2 weeks ago

Awesome, thanks!

On Wed, Jun 26, 2024 at 11:56 AM Matt @.***> wrote:

okay, I should be able to get to it this afternoon

— Reply to this email directly, view it on GitHub https://github.com/pangeachat/client/pull/319#issuecomment-2192056764, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYPKFJSIJCZVTKIA2OCTQLZJLQDXAVCNFSM6AAAAABJE5ATG2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSGA2TMNZWGQ . You are receiving this because you were mentioned.Message ID: @.***>

-- Will Jordan-Cooley CEO and Founder Pangea Chat https://www.pangeachat.com

casualWaist commented 2 weeks ago

These changes seem to work. Let me know of any issues.

ggurdin commented 2 weeks ago

Hey @casualWaist , thanks for making those changes! Everything looks good, except on line 517 of choreographer.dart - when reading that value, it's potentially null, so reading it into a non-nullable bool causes an error if the user hasn't set it yet.

casualWaist commented 2 weeks ago

Sorry about that, let me know anything else.

ggurdin commented 2 weeks ago

Looks good! I'll resolve the conflicts and merge.