GitLab Code Review Weekly Workshop, 16 Sep 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Sep 16, 2022

Description

In this session we discuss approaches to community contributions and identifying testing gaps. We also pair on a gitlab-ui MR review.

A

Good recording.

B

Thanks for uh joining us for the code review, weekly workshop um yanic's got something to show and tell and or a question so what's going on, yannick.

C

um Yeah, hey everybody, I've, probably I've got all of the you just mentioned a little bit. um What I'm? What I'm bringing along today is um in our last meetings, we've been speaking about um merge, requests that that include graphical queries and that they're a little bit tricky to to test and review, and all these sorts of things um the dmr looking at here is a community contribution um and, and my eye is an astonishingly good one and on one hand the changes are pretty much straightforward.

C

We are having an already existing feature which is using the rest api and it is now refactored to be using graphql queries, no more features added, just a refactoring, pretty much pretty straightforward. So that's fantastic! I reviewed this the way I reviewed things which I'll be telling you more about in a second approved. It gave it to another maintainer because uh without any comments like I was just like okay, this is how you do this. This is just perfect turns out.

C

Not quite so, there were other people involved and I was very happy to to have them around um finding no major things, but still a couple of them.

C

So um I think this is something worth mentioning or with bringing to this meeting, and we could now approach this in a couple of ways um and whatever you feel comfortable with, we could option one just start out giving this a review without looking at all the things others have found and see how far we can take it, or we just basically can kind of um expand all the threats and see what is happening there.

B

So I'm I'm really interested in your thought process of what were you looking at where you concluded this was okay and then what would be like the lessons learned after it sounds like you've got. You've peaked my interests and, like you, the story sounded like just every other day, but then darkness.

D

Into the land and.

B

I gotta, I gotta, hear the rest of the story.

C

I'm very very happy to do this. This way, um full disclosure- these things that I mentioned in here, I have not yet fully understood, which was probably part of the uh a part of the thing that why I didn't mention them so, um but I'm I'm happy to to tell everybody what um how I approach this. So um as a quick overview, we are looking at the issue analytics request for projects and groups.

C

If I remember so, it is pretty much straightforward if you're speaking in the rest world, making the requests getting the data displaying it graphical kind of the same, no, no, no mutations, nothing crazy, pretty much getting better showing it. um So I kind of deal with that. First thing: I did um manual testing everything's, looking fine, all the features are there. um As far as I could tell this has been working. So therefore that was that was pretty pretty good um checking that that will be my very first go-to.

C

My second go-to would probably be the tests and therefore- and that is also kind of probably the trap- I fell into- um I've been looking at the test and and checking for what changed. Is there anything missing? um Is there anything significant going in there and what I definitely first encountered is the the test suite really didn't change that much um other than obviously we've been feeding a get request into it, and now we're mocking all the graphical things and these kind of things, but other than that.

C

Nothing has really changed like in terms of functionality. This still was a um was a full box of all the things that we had before. So I was kind of already happy with that. um When I see that no no tests are fading, no tests have been removed because I did not.

C

I was not convinced that a graphql refactoring requires any more testing they will already be having within our rest implementation.

C

That was my bold statement um so having the test the test suit covered, I was basically just looking for the the actual implementation.

C

If I can see anything that makes me raise an eye bro, but it was most of the things were kind of um this query or basically implementation code to to actually trigger the query um keys that have been renamed things like that, nothing to nothing to really pay too close of an attention to so that was kind of my my first approach, anything that makes you raise an eyebrow.

B

Well, I'm just I I gotta hear what happens next. You said, you won't believe what happens next and I I gotta hear what happened.

C

Okay, so um yeah, that is hence my first comment like I was sold on this thing: okay, nice, great, congratulations, kate, great contribution, very impressed, all good. The fantastic dave then took over, and here is what I feel like. Okay, I need to work on my revenue skills here, because I don't like this minor suggestion. Well, okay, praise yeah! I probably should have done this as well, but other than that a potential testing gap. Let's see what dave has to say- and this is like disclaimer. This is the part where I don't fully understand.

C

What's going on as well, should we also test for type project and check that the query gets past the correct values? I think we can retrieve the variables by doing something like rapper and yeah. Don't worry about this too much now, but um help me out.

C

What's going on with this potential or realistic or actual test gap.

B

So um going to the this is likely the way I'm going to interpret this situation, and this is a lesson learned for whether your front end or back end. This is something just keep in mind, so the intent here would have been pure refactor.

B

No new features we used to do rest now we're doing graphql and this component pure refactor. um It's a big assumption that we have proper coverage to start with, and so that's likely what what dave is catching on to and so one way to to test. So you can either look at the mr as an absolute.

B

What is the health of our code absolutely and what's the health of the code delta because of this, mr and if you just look absolutely it'd, be okay, I'm touching these lines. Let me just start removing lines and do tests fail, like that's my favorite way to test if there's code coverage or not and that likely they've dave might have you know that's rather than looking through the tests like you kind of described, that's maybe what dave did was. Okay, we're doing some sort of like conditions.

E

B

Which is a little different, so let me just check if we've actually tested them by removing them and running the tests and seeing if they fail. That's that's how I know that I might have and I've at times stumbled across a reduction in coverage, but then it brings up the big question of. Is that like a blocking issue, because did we have this coverage before?

B

Is it in scope kind of right? But I would say I would argue uh we're touching it so whatever we've touched and changed should be well tested, because we want to ensure that a refactor is good and working and like as expected and continues to so I would, I think, refactoring does imply if there's no test for something you get to you get to add tests now.

C

I personally feel, since this is a community contribution, that I'd probably be fine without them, but that is a night long discussion we can. We can have about us and there's more than one opinion. So that's that's! Absolutely fun.

B

But, and I think, there's different levels of it too, but I'd love to hear uh cassio or drew or andre. What do you think on this situation of uh refactoring and identifying pre-existing testing gaps? How would you handle that in a review so.

D

For my perspective, it basically depends right on many factors, one of which is uh what uh yannick mentioned versus community contribution uh versus the uh github employee uh contribution in general. What they try to follow is the voice code rule, so you should leave the code base or whatever you are touching in a better shape than it used to be uh so definitely if there is the way to improve things that is not very cumbersome to still do as a part of the same earth request, then we should do it.

C

I'm breathing heavily so.

F

um I reckon like, when you say it's a community contribution. This is a contribution from someone from the community not from libya.

D

F

Okay, and so they touched on a code that doesn't have a test coverage. Yes,.

B

It has, it has some level it's like sounds like there's just some holes in it.

F

Right, okay, I reckon I guess um for for gitlab people to when when they touch on the code- and there is no test coverage, I guess uh there is a bit of a conversation. I mean sorry, I I'm very new to gitlab just uh discussing right now, yeah. So generally speaking, in my opinion, I should say um like if someone has, if it's a very crucial path- and someone has the time to add the test, then then definitely we should definitely encourage people to add tests.

F

But if it's not a very crucial path and- and the deadline is very short, then.

F

Generally speaking, I would still say we should add the tests, but I guess then we can kind of let it slide to some degree and uh hoping that the person would come back when they have more time to add the tests. But uh for like a community- um a contribution I would say I don't know why, but I I feel like I would be a little bit stricter like they have all the time.

F

I think we don't pay for that time. So they have all the time in the world they want to contribute. So they should contribute something good.

G

B

F

B

Love it uh that's so funny um I had not that's. That's, usually not the perspective. I have on community contributions. um Usually it's like you've donated your time. Thank you so much like. I don't want to ask for more, but I love your perspective like what else are you doing.

B

I'm just joking, that's really that's! I think that that's, I think both both of you touched on. um You do have to apply discretion here. So you can't be over prescriptive because some things are like hey.

B

Things are broken on.com, and this is a very fragile piece of code that we know fixed this, but we can't add a test because that's too complex we'd really like to fix.com right now, like that's a big deal for yeah, we should probably do that and defer figuring out how we can add regression testing if that comes up. um So those those trade-off decisions definitely need to happen uh at times.

B

um I will say when something isn't urgent with mrs uh it's it's worth a cycle of just keeping that standard of quality uh as as high as we can. It's worth one more cycle like, and in this case I think david catching that, like that's great yeah, that's that was a really good catch and um I'm glad natalia jumped in like yeah. We don't want to use rapper vm, so natalia was able to I mean this looks like awesome, asynchronous collaboration, david identified testing gap.

B

Natalia had a great idea for a better way to check it out, uh and um what david did then is. This is my number one way favorite way to help contributors if we can provide a patch for them uh where they can fake a basic review and study it and apply it um every time I've applied a patch, it's really easy to think like oh well, no one's learning, anything or whatever, like I've heard.

B

You know some people comment about patches in that way, but uh I think like well over 99 of the time I provide a patch uh the person that applies it. You know they're they're, now reviewing looking over it and owning the solution and uh that's a great way to communicate um changes. It makes it easy for the contributor so the way david applied. The patch was awesome. um Yeah.

G

C

And regarding the um we're getting the pictures.

B

Cassia, I see your hand is up.

F

I I just wanted to ask um by patch: do you mean someone from gitlab jumpscene and basically adds some code on top of the contributor?

F

Is that a patch? That's.

B

A really good question, so it's this comment that david left, which is this big code block, which is actually a diff, so you can actually hover over this code block and copy it onto the clipboard.

B

uh Sorry, can you go back to can go back to the, uh and so what the contributor to do is actually copy this with clipboard and then through the terminal do something like pb paste, pipe git apply and it'll apply the patch locally, so the change is still coming from the contributor, but the reviewer and the maintainer is it's kind of just giving a is giving a large, not large, but like it's, not a trivial code suggestion it's just like here's, a code suggestion and uh so yeah using patches.

B

I highly recommend um getting familiar with generating patches and applying patches as we review code. um This is so this is I I I do it because I I could spend an hour trying to word something just right or I could spend five minutes of just this. Here's the code, I'm suggesting what do you think and now we're just collaborating like on the code and that's great.

D

Isn't it actually something that is already integrated into this uh review flow in gitlab.

B

The suggestion.

D

B

Yeah, um that's a good observation, um be.

C

It a little careful with those like they work, fantastic for small changes like one-liners and stuff, but if things get too too complex, it is at least a little painful to use, um and it could you could easily introduce things that you there was no intention on introducing. So that is what to keep in mind.

B

It's um they. The suggestions, don't work under the hood does not get patches working under the hood. It's a it's a range of lines and then code we're going to replace on top of that range, and so that's they're, they're, somewhat limited in that sense. For one liners like this easy easy and that's great, um I actually end up just sending patches all the time because I like patches so much and I'm a little bitter that we didn't use patches for suggested changes so.

C

And you can have easily one pad for changes in most multiple files, and things like that, so it is regarding the context. um It has a lot of benefits yeah, but.

B

You did identify a great feature, request wow, that was I'm gonna, pretend that didn't come for me so that I could say we got. You know lots of people.

C

Were suggesting this multiple file, patches or suggestions.

B

Right suggestions that actually support the patch syntax would be amazing, yeah.

D

That would be fantastic, actually do.

B

Agree: yeah wow, it's three people that both I think that that's great I'm trying to I'm gonna, create an issue and get all the thumbs up on it. That's how.

G

I guess we prioritize thanks all right.

C

E

I've I've been on the contributor side of this problem like when I'm I'm refactoring something somewhere and verify I'll come across a test gap. I almost always send the new test coverage in a separate, merge request, because the reviews are so much faster, like it's really easy to review one test added to a section of code and it's also way easier to review a refactor when it is a pure refactor, and I can link link out to the tests that say: here's why this reactor is bulletproof.

E

These are all of our outcomes and I'm not touching them. So you know the refactor is good. Both of those reviews are like lightning they're great. um So I highly like I haven't had this with a contributor yet, but if I did, I might even encourage them to do that, because I think the the diffs and the conversations are super clean. That way.

B

Yeah, that's a good, that's a really good option to consider too and for community contributions. I've often I'll create the follow-up. Mr too, of like okay, there's a big testing thing that I'm about to make a suggestion, for I don't want to ask more of them. I know this will take me, maybe five to ten minutes, I'm about to write the code for patch anyways.

B

I'm just gonna just write the code in a separate branch and create an mr for it and that's. uh I think that is definitely a good option too. If I I call these fast follow-ups and if it has to be, if we identify testing gap, ideally we can follow up on it right away. It's not a deferred 46 000 drops in the bucket follow-up. uh It would be a hopefully a faster follow-up.

B

C

A good point yeah thanks so much for the input folks. I I feel a little better about this now and I'm starting to to realize that my take on this and and please take it with a grain of salt, because what I'm about to say is heavily opinionated and I absolutely might be wrong about this, and also I do think that this is another. This is not a binary question, but um regarding a community contribution, I kind of feel like I.

C

I heavily agree with the the follow-up issue and this should be set, and I'm I'm super happy with the way this has been handled in in this smr, um with basically like really actively helping out the contributor. But I would also say if our code base already has a testing gap, that's kind of our issue and if they are dealing with what they have and solving it within these boundaries, then to me that kind of implies as good enough.

C

It still definitely requires a follow-up issue and it is something we should be um we should be taking care of, but regarding a blocking comment or basically having this enough to to be blocking, I'm not so sure about this, and therefore I also um that's why I uh I started to breathe heavily when I hear the divorce, the boy scout rule term, um and that is may because in my soul, in my past career, I noticed that this thing was kind of also being misused as a little bit of a feature creep as well.

C

So I'm I'm super happy with getting our code based polish, but let's be very sure about the scope we're currently speaking about and if we encounter any further problems or additional problems happy to fix them. Let's keep our mrs small and let's open another issue. Let's do all of these things, that's kind of what I would tend to decide for, but, um as said there's more than one opinion on this.

B

Well, I I think I think that it does. It does depend because, like if you are refactoring tests, allow us to make changes with confidence depending on the testing gap, and you know sometimes you do have contributors just stumble upon. This is actually really fragile, and so we really for us to actually change this. We need to.

B

We need to heart, have a stricter testing harness almost first before we change it because of all the context and stuff going on so like I do agree, you're you're putting weight on prioritizing more than than just. We got to get everything absolutely perfect, but I I uh I do think um even if we want to keep mrs small um we do.

B

Oh testing is really good at also just asserting the current change and um if, if there hasn't been a second maintainer mr cycle like if this is the first maintainer in our cycle, um if someone catches that, like that's great, if this is like the third cycle, when someone's introducing new things, that's yeah, I would agree, that's a little, maybe a little nitpicky. But if this is the first one, it sounds sounds good catch to me, but I don't know, uh but it's okay. If we don't agree on it, um we're we can still.

B

We can still work together. Sometimes as long as you pretend to agree.

G

Okay, I had a great question.

B

uh Can you do you want to voice your question cassio.

F

um I guess it was kind of a follow-up to what you were saying that um most of the time, you would just create a follow-up pr with the test coverage. um I was just wondering what would be our preference. Then it is the preference of gitlab okay, let the pr merge um by the contributor, and then we follow up like what. What if we don't have the time or what we've we don't have the capacity then um it will be. There will be still a hole or vulnerability, potentially.

B

Yeah, you ask a great great question and I would say um the probably: the preference would be we handle it in the semr, but I think what yanik is and others are pointing at is. You do have to kind of judge the fatigue level of the mr and the fatigue level of the contributor, and if this, mr has been stretched out things that aren't critical, we do. We don't want to just continue to add fatigue on everyone involved in emr. We want.

B

You know people to feel efficient with the way mrs are being handled on things. So we don't want things to hang around for a long time, um so I think the preference would be. We handle things in the mr that we identify them unless it's just very, very out of scope, uh but with community contributions, especially just reading the contributor and reading, where uh how long this is stretched out.

B

That can um play a huge factor in whether we decide to follow up ourselves, create a follow-up issue and there's lots of competing goals because one we want to fill the test gap but or we could be inspired to create a follow-up issue that another community contributor can pick up and we do want to encourage more community contributions and some community contributors love like oh. I don't really know what to do, but now I'm really familiar with this change.

B

I just made this I'd love to follow up with another one, because I like making more changes and they'll like doing that if mmrs aren't so fatiguing and so there's a lot when it comes to community contributions, there's a lot of soft things to kind of just read in the realm, and I don't think you can be over prescriptive about it. um But if, if you're, really not certain, I would highly recommend just reaching out and asking a question like david did of here's just a question.

B

What do we think and I love how he labeled a question? He didn't label it uh that was really appropriate for for what this was of. Like he's we're just asking a question: if it wasn't gonna work out, then it's not gonna work out and um because it doesn't seem like it was super critical.

C

Do you agree, awesome, yeah, thanks everybody for for sharing your thoughts on this. This was super insightful um that was about. um That was basically all I had to say.

B

And I feel, like you, click baited me because you said like you won't believe what happens next and I was like I was like picturing what's gonna happen next and small testing gap was not what I was anticipating.

C

Okay, okay, okay, maybe the term test gap is triggering me more than than you. I I kind of panic a little bit whenever I hear it. Somebody.

G

Realizes, oh wow, okay, test caps,.

C

B

How long have you, how long, how long have you coded on the gitlab project, I'm joking yannick, I'm doing a slight job at the gitlab project.

B

uh Cool all right, I have a small one to share. uh Let me uh get ready to share my screen by closing out um some other things.

G

uh Here we go. Oh man all right. Let's.

B

B

So this is actually the one we started pairing on last week, and so um it got merged this week, which is great. This is an mr on the get lab ui project and the main takeaway for here um I wanted to highlight was um we identified the original set of changes and it was hard to tell why we were making this change until in a comment below we saw a link to an issue when we linked to the issue.

B

uh There was pictures of of the actual bug that we were trying to change with this ups, that that shows up the gitlab project with the upstream great lab ui project, um but the original state of it didn't really have so in the gitlab ui project, like we have these stories, where we'll for components where we'll actually visually screenshot, um the uh um the rendering of the component we'll do a visual regression test at this level, which is great, but the changes weren't actually like testing these things, and so it wasn't clear if, like we were actually fixing the bugs like the changes to our story, uh and it was really confusing what kind of why we were making the changes to the story, and uh so I left some.

B

I left some questions, uh one of the questions being um here. We were doing this. This is a view. uh This is a view, behavior feature that we don't use a whole lot of where we can use this dot. Sync modifier, and that was really that was strange for me to see. So I just left the question about it um and then uh seeing some other things that are just like these are little. These are a little not what I'd expect. So I left some questions, but my biggest question was.

B

Where was it, I can't find him. Oh yes, yes, I wanted to confirm these things, uh and then I asked the question if we should add a story to actually test that thing, um but it kind of ended up changing what all of our stories look like. Just asking these questions because I was confused and I just want to encourage everyone when you're reading through code and if it's, if you're confused on.

B

Why, like that's a really good signal, and it's worth asking a question about um not just silencing that voice because, maybe oh well, I guess I guess this is here for a reason, um just ask a question about it and it could pay off and yeah. This wasn't behaving like like, I thought it was um and yeah we weren't really introducing tests for what we were fixing. uh So it was really.

B

I was really happy with the the next state of it and it made a lot more sense, so asking questions when you're confused um can really pay off. That was. That was a bit I wanted to share.

B

Thanks for letting me share that, um well, that's it for show and talent questions um now now we're entering the code review pairing part of this workshop. uh Does anyone have any, mrs that they'd like to get reviewed and the call that we can?

B

Let me rephrase that anyone have any mrs assigned for them to review that they would like to collaborate where we can all pair review on it.

B

I always have mars. I always do, and I love it when I get my work done and do this meeting, um but I I feel greedy and selfish doing that.

G

So I'm gonna pause.

B

Okay, I'm gonna. Do it all right?

B

Let me share my screen and let's see what mrs I have assigned uh to me.

A

Just got done clearing my cue so.

B

That I always make sure I don't do any reviews until after this meeting, I'm joking. That would be bad.

B

This is a version bump. um Oh look! It's the same contributor.

B

um This is looks like there is a bug in one of our gitlab ui components that glitches in safari uh that one might be I'm hoping is somewhat trivial. um This is a review on the customers project, which is technically a private project. So I'm not gonna.

B

Do that one right now, um these these two are always in my queue so that we can ignore these two.

B

That's all that's how my to-do list works like some of my email, inboxes, like you know, if I, if I get to a thousand, that means I've, that's zero for me,.

B

Oh, this is also customers. So what do you think we got this version bump one? We have uh this ui component fix one, and I just reviewed this one, so I actually need to unassign myself from this one.

B

Let me double check uh this. One was interesting and there are definitely interesting finds, um but I had already reviewed it. I can go over the the finds that I found. um There's lots of lessons to be cleaned here or we can do something brand new. um Someone speak up. Tell me which one I should do. The version bump one or the ui component fix one.

D

The ui glitch looks interesting. It.

C

Does look like I mean anything but.

B

Okay sounds good: oh wow! It's just one change great okay, interesting. What is this on issue boards and safari? Sometimes the drawer turns all gray and strange turns the color of the box shadow that can replicate.

G

B

Over the close button on a what is that, for I'm.

E

100 gonna start writing in issues that things turn strange and that's the problem I'm addressing. I absolutely love that and um you can all look forward to me sending that in my issues and work requests.

B

Things turn gray and strange. I love that.

A

B

Is really good, uh um let me sign in over here and see if we can recreate this issue, I'm just curious, because that seems so. Wild um props for uh I'm gonna pause sharing. My screen props for.

B

Simon using safari.

B

I was about to resume sharing my screen, um but that's after typing, in my after having uh one password all open, that would have been the opposite of.

G

What I wanted to do.

B

All right signing in here we go all right signing in to safari all right, hopefully gitlab's, not like netflix, and will kick me out for having too many devices signed in.

G

All right- let's see here, I'm here.

B

I don't see the strangeness, but it's doing a draw. So I wonder if this is about epics and not merged.

A

In the description.

B

Oh, it says issue boards. Yes,.

G

Check out issue boards, thank you for noticing that I don't see it.

B

Oh, I really want to see no strangeness.

A

There's no strangeness.

G

Except this, oh gosh.

B

Okay, all right, let's get away from that yeah, I'm not seeing the strangeness hovering over the close button on the label.

G

E

Pop over into the issue and see, if there's any other.

G

E

Instructions about that yeah.

G

And the kanban, if I had an issue and then click on other issues, instead of updating with the click.

B

Add an issue and then click on other issues. Oh.

G

What that's so wild when, if I add an issue and then click on other issues?

G

D

See what happens there.

G

All right, I am on safari, I'm just I'm gonna create a random issue here.

C

Am I allowed to leave a general piece of advice that nobody asked for.

G

C

When somebody ever encounters a bug that is kind of that hard to reproduce on any of those or video really really helps a lot. Oh yeah, yes,.

B

Problem validation um experiencing any strangeness.

B

Create the issue, but then I click on another issue.

G

B

G

D

Well, yeah, but then added power over the x with the close, the button.

B

I hover over the x of this one yeah.

G

D

The labels, the.

B

Labels, I think I wonder if this is a this- is fixed. I wonder what version of safari I'm on. Well, it's about safari yeah, I'm in 15.6, and this is 15.5, so that might be.

A

Well, looked on.

B

A

He can't reproduce when.

B

There are no vertical scroll bars, oh my gosh, how do you get no vertical scroll bars.

D

G

A bigger screen, I guess so let me oh.

B

No, I'm removing labels. Sorry all right! Let me let me not do that. Let me yeah, I'm just gonna, I'm gonna, slowly, step away all right, anyways, there's a bug. Some people.

E

Rip three labels off an issue and walk away from it. Paul that's totally fine.

B

I'll have I'll have to review the recording to recover what the labels were. um I don't remember what I just clicked on. If I was I'm sorry, I think.

A

We just scrolled past the comments where someone also couldn't read. Yes,.

B

I couldn't reproduce this yes, but code looks good to me. Let's see what we're changing.

B

G

G

I don't know I mean.

B

I'm not eager about hacky css things for issues. I can't.

D

B

D

Like to comment, if I may is, I think it's really good that the comment is provided for some obscure css. Yes, because it really helps people to understand why something is there, which is not obvious. However, in this particular case, it would probably be great to actually either link an issue in the comment or provide more specific context, like a version of safari, for example, but the link, I think, would be best.

G

B

Safari bug were box shadows, visible above the drawer when hovering interactive elements. um My biggest question is like: would this affect zn, so getting technical with it? Well does this affect z index stuff? So, like these these these um I've, I lost that the drawers need to stay in front of everything, something like translate. Z. Zero sounds like I don't think it's gonna affect z index stuff, but it sounds like it could.

C

I'd be very surprised if it would, because c and x, kinda is about stacking elements above each other and translate is more about the the x and the y scale. So you basically just move things around. That was a terrible explanation, but.

B

Yeah, so z index is about just the rendering ordering the stacking of stuff, but translate z is about it's another.

C

Dimension, that's what I'm trying to say.

G

C

But still I'd argue that well, we have to to deal with with what we're having but um translate zero is it's supposed to be doing nothing? So it's basically just a reset. um So this is. This is clearly a browser bug and if it's, if nobody can reproduce, then we shouldn't. We shouldn't merge this thing, because there will be a fix, a fix or a hack for browser buttons, no longer existing or maybe already fixed.

B

Wow looks like with preserve 3d. You can use the z property of translation to respect elements on top of other elements like it sounds like that. Would.

G

Change this um wow simon, can uh seems poof man, so weird.

G

B

I'm gonna ask that question. It's very important that gl jor stays on top of.

B

Other things is it possible. This affects the z index at all, looks like.

G

B

To this, I'm I'm not ashamed to to reference that overflow. It looks like um like.

G

B

Trigger this figure c.

G

B

See my relevant significant other.

B

That overflows, my significant other I'm joking.

B

Yeah, so I do want to ask a question: uh is uh this reproducible on the latest safari? Maybe this is a browser bug.

G

What do you think.

G

D

Clarify what sorry uh clarify what you mean by the browser back, so that kind of implies that then we shouldn't even fix it right.

B

Okay, maybe this is a uh maybe this is more of a.

B

Safari problem than a gitlab problem- probably not I don't know.

G

C

And since this is safari- and it is to me, it feels like it might be a little closer to the os than other browsers, it might be worth also asking for the osx version. They're running.

G

Sure I realized maybe there's more that's a problem problem which.

B

Could already be fixed and I get that problem yeah uh all right, while I'm here I am going to throw in andre's suggestion of um wow safari is so weird I'm doing this in safari gosh. It's so weird. All right suggestion could we add a link to the originating issue. Oh, let me say phrase thanks so much for leaving the comment. We had a link to the originating issue um to.

G

Make this clear all right.

B

I think it's good yeah, all right, um yeah that'd, be interesting, that'd be interesting, and I so here's some things that make me feel a little better it'd be really lame. If all of a sudden like the drawer wasn't, you know visibly where it needed to be, um but our capybara tests do test for the visibility of things it clicks on.

B

So we get not visual regression testing, but we do get like some visibility testing on things like this, and so if, under certain circumstances, we had a test and it wasn't able to click on the drawer, because the drawer is now behind other things that those tests would actually fail. I would still be really interested in testing this, because this is a high traffic feature. What's the worst thing that could happen is all of a sudden. This isn't visible anymore.

B

That would be not great um one of the reasons for addressing this is uh I mean I guess this is opened by. I don't know if this is a um I I imagine this issue probably is opened by a a member of the community, and so we do want to prioritize those, and we do say that we support the browsers, even if they have an internal issue.

B

We want to try to get our experience working nicely, so we can't just excuse it, um but I feel good about the question we asked and if and if simon comes back he's like oh yeah, there's no issues with it, I'm probably going to want to just smoke test it locally or with an integration branch.

B

So that's one thing with these get lab ui projects. uh I would want to test out the gitlab project using the package from this. Mr.

B

To just verify that an integration where we're we're good and there's no issues with drawers so um yeah, that's really interesting. uh Thanks for reviewing that one that turned out more interesting than I thought it would be. uh I think that's I think, that's all we got uh does anyone have anything they want to sneak in.

G

All right well.

B

Hey thanks everybody for taking time to share your thoughts, wisdom, expertise and questions, and I think it's good. It's all generally valuable. uh You all have a great rest of the day afternoon evening, adios.