Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect selector causing the MR button to not show up in GitLab #158

Merged

Conversation

RicePatrick
Copy link
Contributor

Description

This pull request fixes an issue where the "Open" button didn't appear on merge requests in GitLab

How to test

Build the extension using instructions on the README, then upload and test. Example of test:

image

Documentation

None

/hold

@filiptronicek
Copy link
Member

filiptronicek commented Sep 25, 2024

Hey @RicePatrick and thanks for the contribution. Could you describe the scenario in which you can reproduce the issue of the button not showing up? I seem to see it just fine

I assume there may be some version discrepancy between gitlab.com and your self-hosted instance? I will try on gitlab.com with your changes.

image

@RicePatrick
Copy link
Contributor Author

@filiptronicek - We actually use gitlab.com (which is where that screenshot is from as well). Here's the same screenshot with the extension installed from the store:

image

The main issue is this part of the selector: div.detail-page-header.border-bottom-0.gl-block.gl-pt-5.sm\\:\\!gl-flex.is-merge-request - that is-merge-request class isn't present in that div anymore:

image

I tried to make the CSS selector a little less "fragile" by not traversing through so many intermediary divs; I'm not sure if there's a specific reason for that. The body[data-project-id] is there to make sure you're in a GitLab project (that's always present as an attribute on the body), then it jumps straight to the button controls. That way if GitLab changes any of the intermediary divs the button shouldn't break.

@RicePatrick
Copy link
Contributor Author

RicePatrick commented Sep 25, 2024

Also, that MR is from an open source project I work on; if you want to go to the MR, here's the link in a clickable fashion: https://gitlab.com/gitlab-org/terraform-provider-gitlab/-/merge_requests/2125

@RicePatrick
Copy link
Contributor Author

@filiptronicek - Ah, here's the issue. In GitLab, that is-merge-request class is only included in that detail-page-header if you're not using the "Fluid" layout option that you can select in your preferences (https://gitlab.com/-/profile/preferences).

Here's the line of code in GitLab that applies it: https://gitlab.com/gitlab-community/gitlab/-/blob/master/app/views/projects/merge_requests/_mr_title.html.haml#L15

So if you change the Layout Width from "Fluid" to "Fixed", the button appears properly with the extension. I just tested this PR's update against both Fluid and Fixed, and it works for both settings, whereas the version in the extension catalog only works with "Fixed".

@filiptronicek
Copy link
Member

Thanks for sharing all the context With "Fluid" turned on, I could indeed reproduce the breakage on any MR.

Copy link
Member

@filiptronicek filiptronicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the button injection as advertised and code looks good. Let's :shipit:

@filiptronicek
Copy link
Member

@RicePatrick please keep in mind that we'll need to make a new extension release for this to go live. I will most likely do that some time tomorrow

@filiptronicek filiptronicek merged commit c2e6a90 into gitpod-io:main Sep 25, 2024
@RicePatrick
Copy link
Contributor Author

@filiptronicek - No problem at all; I figured there might be a slight delay to get the updates on each of the "stores" :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants