GitLab Pair Programming - Frontend, 8 Jul 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: FE Pairing - 20220708 - Fixing ESC to cancel emoji dropdown

Description

In this session we investigate solutions to an issue with ESC triggering the "Cancel comment" modal, when it should just trigger canceling the emoji dropdown.

MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91829

A

Hello, everyone and welcome to this week's pairing session.

A

The issue I am bringing along so um this milestone, I'm participating in the beautifying, our ui initiative, and there is one of those bugs um that I'm currently looking into, and it is um pretty pretty easy to to reproduce and let me quickly share the screen so start a video it is about when you are definitely issues and pretty strong merch requests as well.

A

When you are going to answer um a comment and you're going to be handling with the emoji picker and you're going to start it, and now you want to get out of it using the escape key. It will not only close the emoji picker, but it will give you the model that basically to kind of dis, discard your changes and quit commenting which is hugely annoying. So that is the thing and while diving into this this part of our code base, I am sure it is safe to say that this can be considered legacy.

A

It is what I then learned um done by a plug-in, which I think it's called add view js, and we are including this as an uh yeah as a dependency. Basically, it's it's done in coffee, uh coffee script. To my large surprise, um so the thing with that is the emoji picture. It is not part of our view application, so therefore it is basically just inserted into the dom and positioned absolute honest. Let me let me see if I can reproduce this somewhere, so we can inspect this.

B

So my initial thoughts are.

B

You're in the text area and you press colon and then the the drop down appears. um The active element is still the text error.

B

So when you hit escape um your uh whatever key down or key press it listen, you've got listening, is gonna hit all the listeners on the text area, which includes the cancel, writing comment thing.

B

So the only way I can think of to prevent that would be to make sure that the gfm or whatever app.who js listeners bind their events before the view. Application do and then call stop immediate propagation when you get escape, but that is horrible. That's a horrible mess, so I hope there's an easier solution, but that's mine.

A

Just to get us right on which of the two event listener would we have to call to stop propagation on the emoji picker listener.

B

Yeah, exactly because.

A

B

Think the event must be attached to the text area. I think.

A

I'm I'm not quite sure if this, because the problem is that, basically what we're, um when the the view application, the text area is being edited, which is a part of our view, application and- and it has an even key listener on the escape key. We kind of wanted like this is. This is not a bug. This is a feature we want this. We want this to work, it's just. The condition is going to be always work, but if the emoji picker is open, then just please ignore this.

C

Yeah that could work so.

A

That that is what I currently came up with like this is this is heavy work in progress and not going to to be a proper, mr, but um so this is the. This is the event handler for the um escape key for the for the note form- and this is then just um checking if the if the emoji picker has a style blocked to it, which this, to my surprise, it works with quite a high reliability.

A

It's still rubbish, but um just to basically as a prototype. This is this is currently where I'm at with this.

D

I yeah I I agree with what mark's saying it'd be nice if we could lean into our data structures here and we didn't have to couple into the note form needs to know about at view, and things like that. If we could have a a stack of listeners such that the latest one that pushes the listener is the first one that handles it, that would be sweet uh yeah. What would you want to say mark.

B

So I have another idea: it's less bad um than what I suggested at first, but it's still bad.

B

um The view event listener key press key down event listener, whatever it is, um could uh wait a microwatch or next stick or whatever, and then check if the event was prevent defaulted, so that would allow whatever other listeners there are to interact with the event object and maybe prevent default.

B

So, for instance, the outdoor who listener could do event pro prevent default if escape was pressed. And then, when the view event listener eventually runs it'll see that it is default, prevented and not run its own stuff. That's also horrible, though I don't.

D

Like you still throwing it out there, that would sorry yannick. uh Am I really choppy? Is there anything issue any problem with my internet connection? Do I see him.

A

You cut it off a little yeah. No, it's! Okay! All right.

D

I'm just gonna lean into that and start interrupting. You all um do you uh that would work. If I think the problem with the emoji picker event handling is it's possible. The event handling is not set up until the emoji picker shows up.

D

Do you know what I'm saying mark so what we would need to do to have actual control of the ordering of these things is have the emoji picker have some sort of like global initialization, where it sets up an escape listener.

D

And we can give that priority because of the ordering first, do you know what I'm saying mark.

B

Pretty much I mean well almost, I think. Basically, we need to look at where, in our code does pressing colon trigger the thing and then we can work from there. I guess.

B

If that makes sense, just like find where, in the code that is, triggered and then see what we can.

A

Do awesome: let's: let's do this, um the good news in the battles? I've! Definitely I have been there, but I don't have it oh.

A

So let me see if I can quickly refine this m, so I need to figure the exact name of the library which I currently don't have at the top of my mind and within the node modules folder, there will be around seven a file in there um not too big to to get there to get it and there has been a listener in there. I.

D

I think it's called the at hill. I think there's the library.

D

um So, wherever we're, that's that's the library that, but wherever we're using that library is what we'd want to look for.

A

um Is it yeah, yeah, yeah yeah, pretty sure that is the identity function, recording.

D

uh The complete uh yeah so like so this isn't just a emoji thing, because this is also used for mrs or issues or all this stuff right.

D

Does this show up for the other things as well.

A

Yeah, this is basically just this functionality like the drop down. um Emoji picker like um this is all at you.

D

But it also shows up if you're like trying to add somebody right.

A

um I haven't checked but from the looks of it, it really really looks familiar yeah.

D

If you escape here.

A

Same issue: okay,.

A

So are we looking? Are we looking into the call where this library is actually usage? Do we want to look at the source? Can.

E

I ask a dumb question before we move on: please: how would this normally, how is this supposed to work.

A

um I'd say like as a user, I would expect um so if we are not opening this, this add to add hue menu, then just on on uh escape press. This model is absolutely fine, but when we do have the um the text area in this kind of edu field and when I now press escape once, I would expect to only close this user menu for now or emoji pick or whatever it might be, but not go into um not asking me to cancel creating the comment so.

E

How would we do it if it was a like a normal view, app if it wasn't hacked together.

A

Well, basically, with handling state, I suppose that we could. We could have the state um telling us that the the ad view is currently active and, if so, just ignore the key press.

E

A

E

One of the ideas, which was your initial, the idea of the idea that you're currently going with, where you're just checking the state of if it's open or not by seeing if that class exists,.

A

The the hq library will not give us any any view state since it's not a part of the application. What mark supposed already so I understood is that we are listening for the.

A

We will still be listening for the escape key press in our view, application, but we'll be checking on the event if it has been prevented by default, which is something we would need to build into the library first and then we would know okay, um something is trying to keep us away from uh open our normal menu, and this comment is not meant to be cancelled.

A

Did I get this right mark.

B

I wasn't listening sorry.

D

No worries, here's another alternative, because I I'm looking at the code. I see that we might not actually control at his escape right. That comes from that vendor library. I think so another another idea.

D

Sorry, I'm sorry for breaking up. uh Another idea is right now the quick fix that you have in that draft tomorrow is asks the component to tell if the emoji thing is visible, but what, if we told at who, what if we could attach listeners for whether it's visible or hidden, to put some sort of blocking flag on that contain elements container so like what?

D

If that condition in uh the view component was like, uh is out like it checks if it has a class out of control or something like, I don't have control of this whatever, and then I can't I'm not gonna close it, but then, whenever the emoji thing closes, then we just remove that blocking flag.

D

Does that make sense.

A

um It doesn't make sense. The only question I would have is: how will the viewer application know if this thing opened is open or not a little bit on a reliable base.

E

Did you say it was loaded in? Is it a child of the textbooks.

A

um No, it is, um it is just very. um Let me see if I can reproduce this easily, but um it is basically just a touch, the very end of our of a body, tech and then positioned in an absolute race. So it is something somewhere in our dump, but it's not that we can actually deal with it as if a child or something it's just somewhere.

A

So and sorry for one more quick note, because you just mentioned the vendor library, um with the current setup, we haven't actually cut many moons ago, probably, but we already fought this um this library, so it is kind of a vendorous thing, but we are in full control of this part. So making changes to it is kind of a realistic thing to do.

D

Right that makes sense, um no suggestion that I'm, I think, might be the simplest it's still not great, but if at who at who is already given reference to some sort of element, it's targeting, uh and so when it's opened, we don't have to couple at who to anything else and what we could do is have leave some sort of artifact onto that element like a class or a data attribute or something when we're open and when we're closed, we'll remove that artifact, and so the other thing that has reference to that element is the view component and where everywhere else, we're using gfm stuff.

D

They also have a reference to that component and we can test for the presence of that artifact, whether it's a class that was added or data attribute. That was added whether we need to actually cancel this thing or not.

A

I'm not entirely sure if I fully understood, but um I like one of those problems I'm seeing is um that this, um the the dom elements the the edge hue library is springing along. There will be staying in the dom, basically all the time, so here on the right, you can see it and because it's just triggering the start. So if that is, that is why I came up with um this check, actually checking whether or not they're currently displaying.

A

So I'm not sure. If I, if I understood you the right way but kind of what I was hearing was basically check, whether whether or not these things are available in the dom or not and then apply something on on a view, application element. If that's the case, then I do see some issues regarding reliability, because.

D

It's available: can we check out the gsm autocomplete library? Can you check that out in the code or that module yeah? So are there any like hooks for if we're shown or hidden.

D

D

There we do like, I guess, that's our template for displaying things. Are there any hooks for whether we show or hide this thing.

A

This is this sounds like display template, maybe to.

D

Me I don't think that would be it. Maybe not. uh Okay. My suggestion is based on oh there's callbacks. uh Is there a get? What's the get default, callbacks.

A

um Do you have a line number? I'm not! I don't know callbacks okay, gotcha um yep, let's so that should be in that's not to find in here interesting.

A

That's what I would have expected honestly.

D

B

D

I wish there was documentation for this thing.

A

It is it's not super well maintained, unfortunately, even though like for us, oh it's a temporary fork from a couple of years ago, probably.

A

This is this: is the original repository no longer maintained documentation? Here we go callbacks default, provex.

D

So I was wondering if there's like a, I think that there is I'm looking at um looking at these random lines of atmo code random found in the internet. um Oh oh cool, yes, are there? Is there like shown and hidden.

D

C

No there's none of.

D

D

Well, that's lame! Okay.

D

D

D

These aren't necessarily all of the callbacks.

D

Do you have this thing? Do you have this thing uh running right now,.

A

D

A

D

Here we are okay, can we can we do something crazy go for it? Can we just try uh adding to callbacks? Can we just try adding a shown and a hidden and see if these things work absolutely.

A

And where would we add those and the gfm will be able to reproduce this easily? Oh.

D

Okay, so lg one on line 231.

A

241 um ah gotcha um sweet, so callbacks.

D

Sorry, I'm reading mark's found some things um so yeah, that's that is shown. I think it's I think it's past tense is what I'm, what I'm seeing randomly in the internet that might not be related at all in a hidden.

A

So it's shown and hidden.

D

While we're writing this, what what we could do is create a proxy that takes any key uh and so anytime someone's trying to access anything from callbacks. We always console log it and that's one way we can investigate what the api is, but that's too much work right now, uh let's look at it. What's console log here, um just something that we're hidden. You know.

D

I got shown I got I get. I got shown for reals uh cool all right. Can we just to double check too that we're in the right emoji, unlike any of these callbacks being called? Can we add a console log just on one of the other callbacks, like? Maybe I don't know, matcher.

D

A

Let's see um so, let's pop up the console and open the emoji picker and really nothing's happened. Oh yeah matches running matches running.

A

Nope just symmetric.

D

B

I found um in the.

C

B

When you, when you trigger escape it, calls this method here, this uh I've just pasted it in the chat.

A

B

So basic, the context, I think, is some containing element somewhere. I don't know what exactly it is yet, but it's emitting, um I'm assuming trigger, must be the jquery trigger method. So it's trigging trigging a hidden event right.

B

uh I wasn't watching what you were doing. Have you already found this out? Okay, so.

B

I'm wondering if.

B

If we, if we find out what context is in this.

C

B

um Then, where we set up at who, uh maybe we could add an event listener, ah god, that's so nasty okay, we can.

A

Add an event listener, actually fire up an event which we're gonna be catching. In our view, application.

B

Well, when hidden happens, the thing is we don't know why it's hidden? Oh no! We do. We actually passed the events to the hidden uh event as well, so we could find out if it was hidden because of escape was pressed.

B

In which case um yeah, we can do something useful there, like, I don't know, like paul said, like add dataset something somewhere or.

A

um We could like this might be might be a very bad idea, but maybe maybe eventually would be helpful to just like pretty stupidly. Add a glass to the body tag. Basically telling add you.

A

That might be something.

A

Let's, let's see if if this is actually if this thing is firing, because that would be a good, a good hook to to get into.

D

um I I just shared on a chat, a link to code. I found a source graph, trying to figure out what context was, and it looks like context- is actually their quote-unquote like controller, and I think it is triggering an event on the in in imputor, the in in in putter.

E

D

The nputor- um and so this is great, um I think, let's it would be great to see if we can listen to you shone dot at who and hidden.adhoo on the input that we receive from the gsm monocomplete.

A

Well, that would bring us back to this one. Wouldn't it.

D

So callbacks is a different thing. It seems like. I think we we would listen to you. These events from the input dot add to we would do input dot on.

A

B

Yeah, so if you go to where we set up emoji.

B

um So after input who we could, for instance, um do input on uh hidden shown, I think you can add, with a space between them to have both events.

D

But I, based on that code that I just shared, I think these are namespace events. I think it would be hidden dot. I think they're building it with the name and then they're, appending dot, add two at the end of it, so it'd be hidden dot. Attu is the name of the event like that yeah. Let's try to do just the hidden one. We can worry about the shown one later. If we do just the hidden one and then it's just yep, we would just add comma under the event listener.

D

D

We can worry about showing later, but let's just do on and then do comma. It takes two arguments, I'm not talking. Clearly that's. I guess.

A

It gets to know I'm yeah um something along those lines.

D

Yes, yes, yes, yes, yeah yeah and just throw as many fat arrows as you want added. That's sorry.

D

A

Something under those lines.

D

Yeah, let's, let's see if that works, what do you think mark.

B

Why do people talk to me when I'm not listening, I don't I don't get it.

D

What's going on oh, never, mind, forget it! What.

A

Do you think just ask how you like this thing.

D

This do you think that this hooking into the shown hidden and leaving some sort of artifact on the input?

D

Do you all think that's a a decent approach.

B

Maybe there may be a uh a race here like which, which event this sets up, gets set up. First, the view one or uh the yahoo one, but.

A

Let's see that's yeah.

B

Let's this is a good start.

A

So the event fires, that's the good news.

B

Can you can you log the event that you get as well, yep, absolutely.

D

um It looks like data whenever it's the second argument to the trigger that comes from context. I'm looking at the source graph, the code and source graph, uh okay,.

A

How many paragraphs do I need them? It's one.

D

It'll just be one okay, and you can just.

D

What is eve here hide.

A

Checker event type hidden, but that is not too surprising what else can be of use in here.

D

um You're, actually gonna get the key. You might get the key down event: uh you'll get whatever it seems like you'll get whatever event object got triggered that caused this, where there was a mouse press event or whatever they're, just passing whatever that native event object is they're, just passing it through, which is kind of cool, because you could get maybe like the current target or stuff, but I don't think we need that.

A

ah Okay, this this whole thing is the actual event. um I understood yep um yeah the current target, since we are kind of computing. This thing in view land is probably not too helpful. Unfortunately, I'd say yeah.

D

I don't think we'll need that.

A

D

So here's my here's, my idea.

A

D

Be a really bad idea, um but I don't know. Maybe that's the theme we just have. We all have bad ideas today.

A

D

To preparing, let's add another input on where we do shown.addfield, I feel like there's like a like a dr seuss library. uh I don't mean to dis at him, but.

D

Maybe maybe there's some missed potential there uh so yeah. Maybe it's a good idea for us to go ahead and log these, but what we would do on the shown is. We would ensure that a class or a data attribute exists and then the hidden remove that thing.

A

Yep that that could be our um like the the body indicator. I was referring to basically so like, let's see, if that, if that might be good enough, um like.

A

I have not written the previous selector for.

D

A long we could just use the end plate that we want. I would not do it on the body, because it's possible that, like you've, opened up one and then you go to another comment. Maybe in press escape or something like that.

B

Yeah we should. We should keep the context local to the current input right. Basically,.

A

um Where would we then edit? Is it.

D

Dollar: dollar, dollar dollar bill y'all.

A

D

A

Just like that,.

D

Yes, except I'm pretty sure this is a jquery thing, uh so you'll you could just do the fancy. What is it called? What's? How do you do jquery.

B

I think it's a class or classes or oh yeah, add and add and remove class. That's it well done. It's.

D

Just add class.

B

Add class in camel case yeah.

A

Okay, so the class list is not: oh, it's not going to be a trajectory, uh object, gotcha, um add class and remove class.

D

Yeah man, jquery's so clean, so clean.

B

Yeah, we should go back to that. How.

E

B

We implement, we should re-implement all of our stuff in jquery. It's so nice.

A

Taking that for the next beautifying ui initiative, maybe.

A

So, um let's see, how did I call the class attractive.

D

Well, it should be gone now right.

A

Well, is this painful to reproduce this?

A

A

Can we just yeah should rick, um let's.

D

Go ahead and let's, let's go ahead and go to the view, event listener for escape and let's see if we can read it on escape. Let's, maybe that's a let's be optimistic about what this step and just move to the next one yeah.

A

um So that would base it. This would basically mean that we.

A

I'm not looking for this.

D

Yeah uh not uh not necessarily because we can still keep this scoped to inputs.

A

D

So this view component should have a ref to the input.

A

How do I address this one.

D

Can we go down to the template and see if we can find the input.

E

I'm going to.

D

Go up maybe yeah yeah.

B

Yeah, let's do texture, ref text area, yeah, yeah,.

D

That should do it.

A

Okay, help me out syntax wise, well, there's a ref text area.

D

It should be: if first can we um on the cancel handler, can we just console.log this dot dollar refs dot text, stereo.

D

uh Oh gitlands is putting putting metadata in weird places what.

A

Is there we go and this stuff that the text area? That's what we just said right, it's refs um yep! Let me just just for the sake of it, so this doesn't ever sweet. um Let's use the cancel event, then oopsie.

D

Lee's gone man.

D

E

Maybe he's still listening, maybe.

A

So cancer handler is being triggered, and this will give us the text area, and this is exactly what we've been looking for. Isn't it.

B

Does the text area have the.

A

B

No, no, the class.

D

Maybe but maybe we removed it and then logged it.

B

Right exactly so, this is this means that the view event happens after the the the who event is handled.

A

Which is a bad thing, correct.

D

Wait wait, wait.

E

B

So basically, what's happening is the you so you've got you press colon you've got the at who thing open. You press escape the event bubbles; no, it doesn't bubble it. It goes down from document to the text area and then it fires on text area and it hits the at who event listener. First, that.

A

Also speaks down yeah.

B

It removes the class it closes that and then it goes to the next event handler and text area, which is the view gfm autocomplete one which handles the cancel.

E

So why couldn't we just tell it to stop there, I'm not very good on event bubbling well, that's.

D

That's that's a really good point. Stop propagation that'll! Do it! If we do, if we do on hide and then e dot, stop propagation that'll, probably do it because.

B

I think we need to stop immediate propagation, because the events.

A

Are on the same element yeah? So just absolutely do you agree just to get this right, because if we catch and or yeah, if we catch an event that atku actually kind of computes or deals with, then we don't want this event to fire anything else and therefore it's a good idea to immediately stop the foundation correct sweet. um Where would that go? Let me have a check. I'm going back to the source. I'd say.

D

The I think the the auto complete.

A

um Yeah yeah, most likely.

D

Original event that got triggered whether it was a key down or whatever.

D

We can e dot stop.

D

B

D

B

Only want to stop immediate propagation if it's the escape key.

D

We'll only want to do with them.

B

Yeah because otherwise, um well, who knows what else we'll be stopping like? You know, maybe there's other behaviors that we do want to actually let propagate down.

D

Point of keeping this maybe a little scoped. Can we let's look at the details on the log of this event again.

A

The hidden event.

B

A

Sweet yeah- and I I do agree with mark said just for the sake of the demo: let's, let's keep it like that and see if it actually does what we wanted to.

D

D

Is it working it's working.

A

But hold your horses because it has been working for a couple of times before, like this. Is this.

D

Is working, what do you mean what.

A

It's just only answering the comment.

D

What wait, what.

A

Sorry, what's yeah.

B

A

So this is, this is going to be a merch request and if we yeah that might be a good spot to start. Let me just read out the thingy.

A

This might be because this one does not have the modal at least. So, if we're just gonna be adding a new comment, yeah on escape, nothing will happen. That's the thing I'm now pressing escapes for a couple of times. There is no model when you're just gonna be adding a new comment. That's that's! Why we're not running into this issue, but when you're actually responding to an already existing comment, then we are still running into it.

A

Let me wait a second. This was not open. Now it's open pressing escape for once there we go problem.

A

So um stop propagation does not seem to do the trick. Just yet, I'm afraid.

B

Well, I wonder, then, if it's doing it too late, um can you.

D

Can can we um can? We comment out the remove class of add too active.

D

Can we go back to the note form.

A

And see if it's still, if it's still on there, if it.

D

A

D

One more time can we go to the note form, dot view.

D

So this is definitely for reply stuff.

A

um Pretty sure well, the thing is: if this cancel handler gets, um it's called on the on the right escape key, then I would say yes and 255 is actually backwards.

D

If you go back to uh line 255, if.

C

Athlete's active.

D

We don't want to cancel the form.

A

Yep, that's correct.

D

So we can just put a we can put: um let's: let's go ahead and use the refs. We were trying to use. um Oh yeah. That would be fine, but that's fine! That's fine! We'll we'll refactor later see if this works.

A

Will never be a classic, at least so maybe this.

D

A

I only let me talk you through this, so running some words, opening it up pressing escape once no model pressing escape twice. No, no, no, nothing.

D

No that's good, because that's because we're not removing the class at the moment right.

A

100 gotcha sweet one more time, reloading.

B

Well, so we're doing two things here: we're preventing we're stopping immediate propagation and we're doing this event this this class toggling thing do we want to see if we can get get rid of the class toggling thing and just rely on the event um uh stopping propagation, because I think it's not clear which of these is actually fixing the problem if either, if you know both or whatever, let's.

D

Try, let's try the class one and then we'll try the propagation plan. Absolutely.

A

So this is, um I just commented out the the propagation ones that we are now running on the class solution only hitting escape once hitting escape another time working, just fine wow. That is that's amazing, so um putting in the stubble media propagation copying uh commenting out all of the class shenanigans.

A

um What are we going to do regarding the star propagation, with our condition in here? It's basic it is, it shouldn't, be anything required or I'm against.

D

But I don't I agree with what I think we were identifying earlier. I don't think it's gonna work because vue is attaching its listeners last.

A

Well, and we I'm I'm a little bit afraid, even if this is not working, I'm a little bit afraid about race conditions. Maybe that could be another thing.

D

I I think these these things are usually uh deterministic like the event. Okay,.

B

But it's like if a future refactoring somehow ends up attaching these vents in a different order. It'd.

D

Be fragile, sorry.

B

Can you do this read speak out what keys, you're, pressing.

A

Yeah, I'm sorry yeah um um reloading once more, but um just to quickly add on this. um I'm with you mark like this is a a fragile thing, because it'll be hard to document the connection between those those two kind of events in our code base. So if anybody will be touching one of those files, I could easily see this being broken without anybody noticing testing this, probably not to trivial as well. So um writing a couple of words. Colin, I'm hitting escape once everything's fired this route.

B

So that doesn't make any sense to me because um hidden so hidden event fired.

B

Can you add, in the view cancel handler.

B

Oh, it's the cancel, it is there. So can you go back to the console where, where is that cancer? It's cancer handler is happening first, um hidden event, fight right. That says to me that vue is actually attaching its event first, which makes sense. Actually, of course, it is because yeah.

B

The text area is created by view, so, of course it adds the text. It's input handles first yeah.

B

B

Yeah, so, okay, so this this brings me back to my suggestion, where in view, we do this horrible thing where we like do an asynchronous callback and wait for the you know later event handlers to to fire and maybe stop well, not stop propagation but prevent default or something nasty like that. Yeah. I don't know, but on this note I'm gonna, I'm gonna go. I'm done.

A

Thanks for your input mark, that was fantastic have a great weekend. Thank you, see ya.

E

I think something mark said before was like: if the. If, if you is hearing the event first, then it can check to see if something is open and if something is open to leave it right.

E

So like what we were saying before, if, if the pop-up can add some kind of like artifact, I think you said before paul uh to say something is open, and then you can say: okay, if that artifact is.

D

E

Is there then don't do anything yeah.

D

The problem that's kind of what the class approach.

D

um What I think mark was talking about is we you could, rather than having his view, synchronously handling the event it could asynchronously handle the event such that when that asynchronous process actually starts any other event, handlers would have modified the event by either calling stop propagation or something like that, and then it could know. Oh, I need to skip this. So sometimes you can have like an event handler skip its place in line by waiting to process a day, but that's really but you're into timing. Weirdness.

A

So that kind of means, like yeah, hey view application. I know this button was pressed, but just wait another second. Maybe there might be somebody coming up telling you don't actually do this right.

D

What what I like about the class? Oh sorry, what were you gonna say.

E

um So could it not say like um so, the pop-up always gives some artifact to the window of the page or whatever to say the pop-up is open and it also stops the propagation whenever it receives it right. So then,.

D

We were, we were looking. We were looking at doing either one of those we may not do it.

E

If we did both, then the view application could say if you get an event and there's no artifact act on it,.

D

E

Whereas, if either of those are not the case, don't.

D

What I think yannick just confirmed, though, is that the stock propagation was not actually stopping the propagation like we thought so we're not. If we were going to get stop propagation to work, we would have to defer some things in the view thing, which would add a lot of complexity, which we're not sure we want to do.

E

So I thought uh stopping the propagation didn't work because vue was reading it before the pop-up did.

A

That's correct, like.

E

If, if the pop-up's able to give a artifact, then view can say okay, is there an artifact? If there is don't worry about it, leave it.

A

An artifact um for something like a class we're adding to the element, for example,.

E

A

That's that's the other. The first approach we went with, which was working working, just fine, sorry,.

E

So why can't we combine them both and then it will work.

A

um Why do we need? Why not, why wouldn't we go with only a class, or what do you mean by combined.

E

um We would so if we go with only a class and then the event fires um it and then sorry, if we go with only class, the event fires and we don't know if the event has been handled or not yet by the pop-up.

A

D

Correct me, if I'm wrong, but like the class approach, is also assuming a certain order of the events.

A

It's just the other order. The other way around right.

D

And so, if you did, if the other order of events, if that wasn't true, stop propagation would work.

E

D

And it sounds like yeah: let's just call it stop propagation. That's that's totally fine.

D

Ideally, though, we should have a recreatable test for this and these order events are deterministic because of the way like the javascript engine works so like we could just cover all our bases do stop propagation so that either order the events will probably work. But then you run into like it's not happening as other orders.

D

Maybe we don't need that, but I'm I'm fine with being extra defensive, but I think we were all so concerned about leaving too much changing too much here, because it is very legacy fragile code and there might actually be some sort of event that is dependent on it propagating so.

D

If we're, in that case, I'm inclined to if we don't actually need it, let's, let's try not to touch it. Let's try to touch as little as possible with this. So I do. I do think that's an interesting idea of like if the order of events is obscure. Let's do both, um but doing too much could be risky.

A

So, just just to get this right, you are proposing to go with a class only solution here.

D

With definitely a test because then, if event order changes we'll have to address it,.

A

A

D

We could do a feature spec, you know feature. Specs are awesome, lee loves feature specs, um he loves writing them.

C

Yeah, I probably prefer them to to view specs at the moment after, although paul did all the hard work last week, but yeah.

D

We do have actually a really cool. um I I can't wait to show you this. This just got merged in, and the mr is open for almost like two years, but it just got merged in uh oh well. I'll share my screen, then.

C

We can use mousetrap, though, um to to do key, presses and stuff interviews back just.

A

To just to to um get us right as just so we're speaking for the same thing, um a feature spec would be something that would also be considered. Integration tests would basically press or simulate this key press and then see if this thing is still attached to the dom or something. So it's like a fake in the user input correct.

C

Yeah, it would navigate to an actual like issue page and click on like the reply button and yeah really like emulate a user um and we were we normally, I think, sort of inspect the dom to say. Can you see you know, can you see the model or can you see the emoji picker, or can you see whatever we want to check should or shouldn't exist? That's it.

D

That capybara is so good at being able to wait for elements to show up like before. You assert if you're saying this shouldn't be here. It'll wait a determined amount of time to make sure this thing doesn't show up. So that's copy bar is really great, but we do have these like integration, specs.

D

We have one where we're already asserting this is actually in js, dom land opening up the uh um emoji picker by focusing in on that text area and filling it in with a certain input and stuff and then we're starting what our auto complete suggestions are.

D

So we could take a front end integration approach to this, where we're mounting one of these inputs. That would show the cancel modal and we we do this through the jsdom way. uh These could be a little bit more complex than they would in copy borrow. So I would suggest doing a copy bro um might be simple enough, uh but their copy bar is a little bit more expensive to run to, uh but here's another alternative uh of using these front-end integration tests.

D

Just an idea, but.

E

D

What are you thinking.

A

um Not so much at the moment or I'm not not so sure, if I understood it correctly like um how many alternatives are we speaking about like one would be the the integration test, probably the other, that's the part I didn't get.

D

The copy bar specs, the big feature, specs the r spec stuff we could use- or we could use this kind of integration style.

A

Gotcha, okay. Now I see um I don't have a strong opinion on this. um Basically, I would prefer something that is as big as possible, probably and that kind of drives me to what I'm currently seeing, but not uh yeah.

D

So I would use um I would use this as a template for usually how we set these things up. You notice that we're not importing view components or anything we import. um We import the actual, like entry points for these apps and we load them up on um pictures generated from our hamil stuff or just whatever. So we, the subjects here are those app entry points and then we interact with the dom through jsdom.

D

D

So this is the notes app, uh so maybe there's a way that we can get the we can interact through it here. I don't know if snippets shows the this we're just showing the snippets.

A

um You you're sure if this is already emerged, because I can't find a file or maybe I just need to pull I'm sure. It's merged.

D

Yeah, okay cool. It was two days ago.

A

Yeah yeah, oh, maybe my future brunch anyhow, okay, oh, I made a sweet shot.

D

But um it's your call. uh Copyright spec would be cool too. uh I like copyright specs. If you need, if you want any help, writing the test board feel free to uh feel free to ping lee on it. He loves where he needs us.

A

Well, do so I'm trying my very best not to bring this to the next pairing session as well, um but but we'll see how it goes.

C

Didn't you promise at the beginning that uh this one wouldn't this one would be a quick one that wouldn't take.

A

I don't I'm not exactly sure what you're talking about um yeah I um I was. I was expecting an easier solution to be honest, but um still this this was great progress thanks a lot for all the inputs. Do you.

D

Have two more minutes because there's just a couple of things about the solution that are worth pointing out, could you share your screen? That's.

A

Sorry, but um here we go.

D

um Let's go into the note form dot view.

D

So right now on line 255 we're looking at document, query selector. We really want to try to get this from ref's text area gotcha. Let's, let's see if there's a way for us to do that.

D

And I guess it's like classless contains or something.

A

Very selective should still be working on a dominant event shouldn't it.

D

Would it select is that only children, though, does it conclude itself.

A

Let's give it one I'd say.

A

ah Otherwise, you're of the the classless contains right, because it's actually on the on the element and yeah. This actually does make a lot more sense. Absolutely much.

D

Yep and we'll need to get rid of the dot.

A

um Yeah, that would be smart.

D

And we get rid of the equal nulls or if it does not contain so you just throw a knot in front of that.

A

D

No, I mean I mean I mean a banged found at the beginning of the line or the beginning of the. If now you know what I'm talking about? No, no, no, no, no.

E

D

Parenthesis yeah yeah. That's.

A

Sick, it's the end of the week for everybody come on. I.

D

Call the is it the not operator, that's what it is yeah the not operator is that the only operator no minus will that negate something can minus. Is that the only unary operator that happens before the operand.

A

I'm not sure a minus would work in there.

A

So typing some words hitting a column opening up the model hitting escape for once. Let's do it, let's do it. Let's do it we're confined hitting escape twice still, we can confined awesome.

D

Awesome so that's great.

A

Hanging escape a third time still working.

D

E

A

Still all good awesome.

D

We would be screwed if hidden wasn't triggered, so this is all assuming that we can trust at hill to actually trigger hidden when this thing is actually hidden. So it's worth trying out all of the things like. Can we try some other edge cases out? I said a couple of minutes, but let's we had to change any posts. We shouldn't have to refresh anything, but if you open something and then actually click on something without hitting escape on the emoji um open.

A

D

A

Yeah, we should should do this in here, so.

D

And then hit it, that's still working yep.

A

Still just fine.

D

The big question is: can we trust ash hill to trigger hidden.

D

When we wanted to that's that's worth asking ourselves, uh I don't know.

A

Well, I would argue that this is some kind of logic that should be covered by tests in the library.

A

Well, what are you laughing about because you're right, it should be yeah like, but I feel like this is. This is not not a definitely not the responsibility of this. Mr. This is which will go to into the code.

D

This is true: um we want to make sure we're not introducing any further regressions and or any other bugs, which would mean we get in a weird state with these inputs so that that would be just I don't know uh something to think about. I don't have a good answer for it. um The other point: if we go back to the code, sorry- and I don't mean to be taking your time too much, but the other point would be if you go to gfm autocomplete.

D

We want to do this for every at hill setup, not just.

E

A

C

Yeah, because if you've got an auto complete list for like an email address um for contacts or a an app post, a lot of uh assignee or anything um you you don't want it to, and it's funny because until you mentioned this like it's, it's a little niggle, that's bugged me, but I've never kind of considered it to be big enough to to warrant attacking. If that makes sense,.

A

Yeah gotcha um so just to get this right um on every call to the actual library we're making. We want to add this class toggling. We don't necessarily have to then compute it or do something with it, but we want to add the ability that we can actually hook into this. If you want to.

D

We want uh 275 and 284 to apply to every ad who thing um so that is just a there's a number of ways we could do that, um but it's going to take probably a little bit of refactoring of the existing stuff.

A

D

To totally duplicate ourselves, but maybe this is what we do right now. We create some sort of init function that has to be called in every single one of these um or we pipe every single one of these into a common setup thing uh like what, if, instead of line 307, where we call input.adw what if we had a function, that we called that took in these options that we'd pass through to input.field, but then we also set up these event listeners.

D

A

Yeah, I understood- and this, but this by no means this has to live in the in the gitlab monolith right by no means just this logic can go into libraries. That's another.

D

Great question I don't know, maybe we wanted to live in the library. um I really don't know the. I would hesitate to change the library.

D

um But yeah the uh paul yes, you can. You can leave that in the code.

D

uh I would add the prefix, the js prefix cities classes too, so that we're not accidentally styling things.

A

Fair point.

D

And then the other thought is, the other call out is with adding them to everything. There's an ee version of this file, because we have the comments. Con ee, specific objects that we do autocomplete for.

D

And then the last thing is we want to call this. What do we want to call the class? um Do we want it to be at who active or maybe because, right now, the subject of the class is at who? But maybe it's more like input, blocked or something like that, like maybe the subject is actually the input like we want to block something. So just I built your thoughts to, I don't have any strong opinions to it, but.

A

D

A

Just to summarize, what I'm taking away like um first thing would probably be to find a way that this logic should be uh during every call that you add to um let's see about the ee. Maybe that will then sort itself off but we'll see and then come up with a test for this, an integration test hundred percent yeah. That was quick to do. As I said,.

D

A

D

With solving problems in an hour-ish.

A

Absolutely yeah it.

E

Was interesting.

A

Awesome, that's that's letter here, uh yeah, good, fun, good, fun for sure and yeah. I haven't seen it seem kind of coming up this complicated but good fun. All.

D

Right all right! Well, thanks, yannick thanks james thanks lee for hopping on uh you got to plug in your your your laptop yannick.

A

Yep I'll be I'll be out of here in one way or another, but have a great weekend. Everybody and thank.

C

You yeah, I just wanted to say thank you look for last week. We um that mr is hit now so not only did paul improve rewrite the specs using view test details, but we've now got the the reason that came to fruition in the first place was because I was trying to add handling for hitting, enter or return when you rename or add a file in the web ide, and that merge request has has now been deployed to production, and that sounds very much like this issue. Yannick.

C

It's like a little niggle, that's just kind of gets you every time you try and do it and it doesn't work as you expect and yeah.

A

C

Days, thank you very much.

D

A

You yeah, I'm awesome, see that take.