Add Peter Hegman as frontend maintainer of GitLab
Self-assessment
After ~1.5 years at GitLab and ~6 months as a trainee maintainer I would like to propose myself as a ~Frontend maintainer. I have listed relevant reviews and authored MRs below. There are additional reviews in my trainee maintainer issue. Please leave any feedback in the comments
-
Advanced understanding of the GitLab project as per the documentation: - At this time I have authored and reviewed MRs that touch all technologies used in our frontend stack. I feel as if I am an expert in Vue, JavaScript, HAML, Jest, and our CSS/SCSS setup with a very good understanding of how we write GraphQL/Apollo code at GitLab. I have taken on tooling improvements for JavaScript, HAML, and Jest and have completed larger initiatives such as converting the group/project members view from HAML to Vue.
-
The MRs reviewed by the candidate consistently make it through maintainer review without significant additionally required changes. - The majority of my reviews make it through maintainer review without additional feedback from maintainers. I also try to be very mindful of blocking vs non-blocking comments and will opt for follow-ups when possible to move a MR along. At the end of the day my goal is to provide a high quality review but at the same time get the MR merged as fast as possible.
-
The MRs authored by the candidate consistently make it through reviewer and maintainer review without significant required changes. - The majority of my reviews make it through review with minimal changes required. If a reviewer suggests a change I am very open to it. I always thank the reviewer for the suggestion and if I decide to implement it will point them to the specific commit in which I made the change. If I don't agree with the suggestion I will let them know in a respectful way and list out the reasons why I don't agree.
Links to Non-Trival MRs I've Reviewed
| MR Title | Changes | Trainee maintainer issue link | MR link | Comments |
|---|---|---|---|---|
| Display API errors in invite modal before closing | 510-109 | #10999 (comment 599736403) | gitlab-org/gitlab!56957 (merged) | This is one of the more involved reviews I have completed. While this feature was being worked on by groupexpansion it was in a part of the codebase that is generally worked on by ~"group::access" so I had some domain knowledge. I gave a range of suggestions from refactoring Vue logic to improving specs. I also provided a couple patches (gitlab-org/gitlab!56957 (comment 595992122) and gitlab-org/gitlab!56957 (comment 595992137)) to help with the review. This ended up being a fairly lengthy review that went 3 or 4 rounds, but in the end I think we ended up with much cleaner code than we started with. |
| Migrate project VSA metrics to use dedicated project metrics endpoints | 500-456 | #10999 (comment 650591488) | gitlab-org/gitlab!66835 (merged) | This review was non-trivial due to the size of the code footprint. In the end the review went two rounds and I left a variety of suggestions including refactoring a constant, improving spec descriptions, and removing unused code. We opted to handle two of the suggestions in a follow-up as to not block that MR. |
| Convert individual release page to VueApollo | 237-101 | #10999 (comment 533507919) | gitlab-org/gitlab!56882 (merged) | This review was non-trivial as it involved switching the data store from Vuex to VueApollo. I identified some unused code to remove and one small typo to fix. I also suggested updating a spec to assert that correct variables are passed to the GraphQL query. |
| Adds an overview stage to VSA | 212-92 | #10999 (comment 538703670) | gitlab-org/gitlab!56621 (merged) | This MR was non-trivial because the code footprint was on the larger side. I identified a couple of refactors that could be beneficial to the specs and made some suggestions to improve how the spec descriptions read. |
| Adds stage time to the path navigation | 236-40 | #10999 (comment 541909760) | gitlab-org/gitlab!57451 (merged) | This MR was non-trivial because it involved date/time which is never easy. After verifying everything worked correctly locally I made a few suggestions including refactoring a let statement to reduce. I also identified a typo and a place in the code where a feature flag check was unnecessary. I left a couple notes about a possible UX improvement and a backend bug that I noticed. |
| Add mid-term banner | 182-22 | #10999 (comment 609418371) | gitlab-org/gitlab!64535 (merged) | In this review I provided a patch to help suggest refactoring how the modal is shown/hidden. I think this suggestion ended up simplifying the code quite a bit. I also identified a spec file path that didn't match the component file path. We opted to fix this in a follow-up since this was a pre-existing issue. |
| Add "Invite a group" feature to invite members modal | 846-139 | * | gitlab-org/gitlab!50701 (merged) | This MR was related to gitlab-org/gitlab!56957 (merged) and also ended up being a fairly lengthy review that went 3 or 4 rounds. I started out by providing some guidance on how to get some QA E2E specs passing. After that was resolved I reviewed the frontend and provided a number of suggestions. By the end of the review I had opened up ~50 threads. While this was a long review we ended up with a solid MR and it was merged with no maintainer suggestions. |
| Add grid for rotations display + current day indicator | 557-237 | * | gitlab-org/gitlab!49248 (merged) | This MR was non-trivial due to the size of the code footprint. As this MR was pretty CSS heavy a lot of my suggestions were CSS related. I also provided a patch to help explain a refactoring suggestion and provided a few other spec suggestions. |
*Reviewed before becoming a trainee maintainer
Smaller Reviews with No Maintainer Additions
| MR Title | Changes | Trainee maintainer issue link | MR link | Comments |
|---|---|---|---|---|
| Create UserAccessRoleBadge component | 118-43 | #10999 (comment 525549801) | gitlab-org/gitlab!56061 (merged) | I identified 3 issues that would have caused bugs: gitlab-org/gitlab!56061 (comment 525419363), gitlab-org/gitlab!56061 (comment 525419375), gitlab-org/gitlab!56061 (comment 525419371) |
| Add search to group settings | 82-22 | #10999 (comment 528081442) | gitlab-org/gitlab!56238 (merged) | I asked a question that helped identify a false positive feature spec. Also suggested refactoring to a data-testid selector in one of the specs. |
| Differentiate MR Approval Settings from Approval Rules | 69-55 | #10999 (comment 531377893) | gitlab-org/gitlab!54985 (merged) | Made a suggestion to improve a11y by refactoring some form inputs into a fieldset with a legend. Left a patch and before/after screenreader videos to help with the suggestion. |
| Animate the bulk actions section | 77-59 | #10999 (comment 567883995) | gitlab-org/gitlab!60944 (merged) | I identified an issue where this change broke the sticky header. I also made a few other minor suggestions. gitlab-org/gitlab!60944 (comment 567364150), gitlab-org/gitlab!60944 (comment 567364147) |
| Fix load timing with snowplow form tracking | 61-6 | #10999 (comment 624071320) | gitlab-org/gitlab!64822 (merged) | I suggested adding a spec to cover some of the logic that was added. Left a patch to help with adding this spec. |
| Add Calendly invite link to survey response page | 149-62 | #10999 (comment 570536236) | gitlab-org/gitlab!61146 (merged) | I made a few suggestions about removing unneeded CSS and using GitLab UI utility classes |
Links to Non-Trivial MRs I've Written
| MR Title | Changes | MR link | Comments |
|---|---|---|---|
| Move admin user actions into a dropdown | 340-529 | gitlab-org/gitlab!64648 (merged) | This was a larger MR as it included frontend, backend, and documentation changes. It involved refactoring some HAML templates to a Vue dropdown. There were a few other MRs that supported this one: gitlab-org/gitlab!65668 (merged), gitlab-org/gitlab!64812 (merged), gitlab-org/gitlab!64742 (merged). I was able to get this MR merged with no frontend suggestions, 2 doc suggestions, and 1 spec/features/* suggestion. |
Add MembersTabs Vue component |
318-0 | gitlab-org/gitlab!61659 (merged) | This MR added a MembersTabs Vue component that was later used to refactor Bootstrap tabs to GitLab UI tabs. There were no blocking comments left on this MR and in total only 4 threads opened. |
Convert bootstrap tabs on group/members page to GlTabs
|
272-354 | gitlab-org/gitlab!61524 (merged) | This MR used the component added in gitlab-org/gitlab!61659 (merged) to convert the Bootstrap tabs to GitLab UI tabs. It involved namespacing Vuex modules, backend changes, spec/features/* changes, and QA E2E changes. There were two threads opened by reviewers but no code changes needed. We did however open a follow-up to take care of some technical debit. |
| Add the ability to deep link into group/project member tabs | 130-86 | gitlab-org/gitlab!64365 (merged) | This MR built on top of a fairly complicated GitLab UI MR that I worked on where we updated GlTabs was to support deep linking into tabs via a query string. There were two non-blocking threads opened up by the maintainer but no code changes needed and this MR went through smoothly. |
| Refactor members pagination from HAML to Vue | 275-19 | gitlab-org/gitlab!59707 (merged) | This MR built on top of gitlab-org&5616 and required frontend and backend changes. The review went smoothly and there were no code changes needed. There was a question asked by the reviewer that I was able to answer and help the reviewer learn more about Rails. |
| Cascading Setting for "Enable delayed project removal" group setting | 585-33 | gitlab-org/gitlab!57878 (merged) | This was a complicated MR because it required frontend, backend, and spec/features/* changes. It also was focused around setting inheritance which can be confusing. The objective of this MR was to create a pattern that could be used to inherit and lock settings at the admin and group level. This involved creating reusable HAML partials, Vue components and shared RSpec examples. Despite the complexity, the review went pretty smoothly and we were able to get it merged with 3 spec/features/* suggestions, 1 backend suggestion and no frontend suggestions. I followed up with a documentation MR to add frontend guidelines to the cascading settings docs. |
| Add projects field to personal access token form | 220-7 | gitlab-org/gitlab!54839 (merged) | This MR involved creating a Vue component for selecting projects the user has access to. After looking through all of the shared components I wasn't able to find an existing Vue component that checked all the boxes for the design. So I created a new Vue component using GlTokenSelector that allowed users to select projects to scope their personal access token to. The Vue component used a GraphQL query to request the available projects and then display them in the token selector. Because the backend for this feature was not yet setup this MR also introduced a feature flag. The review of this went smoothly and there were no changes needed. |
| Require users to copy, download, or print 2FA recovery codes | 719-45 | gitlab-org/gitlab!49078 (merged) | This was a larger MR that required review from frontend, backend, spec/features/*, QA, and the security team. It involved introducing a new Vue component to display 2FA recovery codes and require users to click Copy, Print, or Download before continuing. Because there were so many reviews needed there were a few comments but most of them minor. Mark Florian gave a great review and left a few suggestions that required code changes but nothing major. This MR also introduced a feature flag and was rolled out slowly to avoid any issues. |
| Add filter bar to group members view | 507-48 | gitlab-org/gitlab!48272 (merged) | This MR was related to gitlab-org&4233 (closed) and converted the old HAML search/filter bar to a Vue component that used GlFilteredSearch. This was a non-trivial MR because it involved frontend, backend, and spec/features/* spec changes. The review process went smoothly and there were 4 suggestions from the maintainer. After one round of changes we were able to get this merged. |
Add parseRailsFormFields util |
273-24 | gitlab-org/gitlab!55702 (merged) | This is a tooling improvement that I worked on. It added some utils to help pass Rails form fields to the frontend. Rails generates name and id attributes for form fields and prior to this MR there was no great way to pass these to the frontend. I collaborated with Paul Slaughter to come up with the best solution for this problem and this is what we came up with. It was non-trivial because we had an idea of how we wanted the developer experience to work but it took several iterations to figure out how to implement it. |
| Add GitLab UI form builder and custom checkbox field | 251-7 | gitlab-org/gitlab!64260 (merged) | This was another tooling improvement that I worked on. It is possible to use GitLab UI checkboxes and radio buttons in HAML files but requires applying a number of CSS classes and setting up the HTML markup in the correct order. To improve consistency of the app and make it easier to use GitLab UI checkboxes and radio buttons in HAML I proposed adding a custom Rails form builder. This custom form builder automatically applies the required CSS classes and sets up the HTML markup in the correct order. While the work in this MR was mostly backend it required a deep understanding of HAML and CSS. After some good discussion and a couple rounds of great reviews we were able to get this merged and I created gitlab-org&6410 to explore future work around this custom form builder. |
Examples of challenging situations
| Type | Link | Comments |
|---|---|---|
| Regression | gitlab-org/gitlab#337156 (closed) | gitlab-org/gitlab!64648 (merged) introduced a S2/P2 regression. This was introduced by missing a bit of logic in a HAML template that did not have any test coverage. After being alerted to this regression I was able to quickly fix (1 day) in gitlab-org/gitlab!67161 (merged) and add a regression feature spec so the bug (hopefully) doesn't happen again. This is a tough one as there wasn't any test coverage to catch the regression. I learned to double and triple check your work and the best thing you can do if a regression like this occurs is fix it quickly and add a regression test so it doesn't happen again. |
| Disagreement | gitlab-org/gitlab!58692 (comment 546124330) | After pointing out that converting a button to div could cause some a11y issues I suggested that we add some extra logic to remedy this issue or find an alternative approach to fixing the bug. The author of the MR wanted to address the issue in a follow-up MR but I pushed for addressing it in this MR as it seemed like a preventable regression. I didn't want to block the MR so I explained why I thought we should address the issue in this MR but said I would be open to a follow-up if it was completed fairly quickly. In the end we ended up addressing the issue in the MR and all was good! |
| Regression | gitlab-org/gitlab#220971 (closed) | gitlab-org/gitlab!32786 (merged) introduced a CSS regression that caused some icon misalignment. I was able to get a fix up the same day and had it merged within a few days. The issue was due to some CSS being used in a place that I didn't realize it was being used. I learned you need to double and triple check all relevant locations when changing CSS. Unfortunately there is no way to test CSS in GitLab. If it is possible to abstract the component to GitLab UI then we can add visual regression tests but in this case it was not practical. |
Once This MR is Merged
-
Create an access request for maintainer access to gitlab-org/<project>. -
Join the [at]frontend-maintainersslack group -
Let a maintainer add you to gitlab-org/maintainers/frontend -
Announce it everywhere -
Keep reviewing, start merging 🤘 😎 🤘
Edited by Dennis Tang