Skip to content
Snippets Groups Projects

Resolve "Customize branch name when using create branch in an issue"

All threads resolved!

What does this MR do?

This MR adds an ability to type a custom branch name in the Create merge request dropdown placed in an issue.

TODO

  • Add backend specs
  • Add frontend specs

Why was this MR needed?

It improves UX of branch management.

Screenshots (if relevant)

To be added.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #21143 (closed)

Edited by Phil Hughes

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • @blackst0ne thanks! I had a couple of comments, but I think I may be confused about what this is trying to do. Specs would help with that, but I understand not wanting to write specs just yet 👍

    I'll pass this to @timzallmann to find someone to review.

  • blackst0ne
  • @blackst0ne Very cool, @iamphill can you review please?

  • assigned to @iamphill

  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
    • Dropdown is too close to the button

    Screen_Shot_2017-10-03_at_11.15.35

    • According to the designs there shouldn't be a divider here

    Screen_Shot_2017-10-03_at_11.15.54

    • The refs endpoint doesn't return all refs. So for gitlab-ce I can type master in the source branch text box & it says it is not available.

    • Does the error state not come in a little too early? Should it be on blur? Other I can type the start of a ref & it'll say its not available but thats because I haven't pressed tab to complete it.

    • The branch name text box always says its available. I type master which I know isn't available & it never changed. I guess is because of point 3.

    • There is a point on the 2 Create.. rows that is clickable & closes the dropdown.

    • Shouldn't these 2 rows have some descriptive text?

    • The inputs don't line up the divider line above.

    Screen_Shot_2017-10-03_at_11.23.37

    • master in branch name & master in source input doesn't do anything. It just fails silently.
  • Sorry for the delay, it looks like @iamphill found a number of UI improvements to make. Thanks Phil!

    Shouldn't these 2 rows have some descriptive text?

    Lets leave them without for now. If the options are confusing to people, we can add it later. They seem straightforward, though, and I don't want to overwhelm the dropdown further with descriptions that may not be needed.

    Another I noticed was the padding of the dropdown. It should match our other dropdowns which use 16px padding on the sides. Right now this one is using 6px.

    @winh Maybe you could also take a look here to make sure this code aligns with the dropdown work you've been doing.

  • Inactive Account
  • blackst0ne changed milestone to %10.2

    changed milestone to %10.2

  • mentioned in issue #21552 (closed)

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 2 commits

    added 2 commits

    Compare with previous version

  • blackst0ne added 3 commits

    added 3 commits

    • d47c80d6 - Remove api_endpoints_paths_helper.rb
    • 10079138 - Merge branch '21143-customize-branch-name-when-using-create-branch-in-an-issue'…
    • 75afc454 - Refactor services

    Compare with previous version

  • Author Developer

    @smcgivern I changed the backend code. Could you review again, please? 🙂

  • assigned to @smcgivern

  • Sean McGivern
  • Sean McGivern
  • Thanks @blackst0ne, the backend LGTM so far!

  • mentioned in issue #39293 (closed)

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    • 4b54c09c - Replace inputs with buttons

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

    • Author Developer
      Resolved by blackst0ne

      The refs endpoint doesn't return all refs. So for gitlab-ce I can type master in the source branch text box & it says it is not available.

      The branch name text box always says its available. I type master which I know isn't available & it never changed. I guess is because of point 3.

      Edited by blackst0ne
    • Author Developer
      Resolved by blackst0ne

      Does the error state not come in a little too early? Should it be on blur? Other I can type the start of a ref & it'll say its not available but thats because I haven't pressed tab to complete it.

      Edited by blackst0ne
  • blackst0ne changed the description

    changed the description

    • Author Developer
      Resolved by blackst0ne

      Another I noticed was the padding of the dropdown. It should match our other dropdowns which use 16px padding on the sides. Right now this one is using 6px.

  • blackst0ne added 1541 commits

    added 1541 commits

    Compare with previous version

  • blackst0ne marked as a Work In Progress

    marked as a Work In Progress

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit
  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 2 commits

    added 2 commits

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    • 6b0b039b - Move ESC listener to dropdown

    Compare with previous version

  • Author Developer

    @iamphill I've changed the code. Could you review the frontend part, please? 🙂

  • assigned to @iamphill

  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • The spacing in the dropdown looks really weird, a lot more than it probably should be. cc. @tauriedavis

    Screen_Shot_2017-10-30_at_10.59.00

    I get this flicker when moving my mouse over the different options:

    2017-10-30_10.58.41

    Button looks like it doesn't match any of our other buttons & also has a flicker.

    2017-10-30_10.58.51

  • Regarding spacing, it should match exactly our other dropdowns. See the milestone dropdown as an example:

    Screen_Shot_2017-10-30_at_9.35.20_AM

    It has both the checkmark pattern for selecting and an input with the correct spacing. The button within the dropdown should not be full width and should match our other buttons exactly.

    See the mockup:

    img

  • If help with styling is needed, please assign to me and I will take a look/push changes.

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • We've missed the merge window for 10.2. @blackst0ne Do you think you'll have the time to work on this in order to finish it up for 10.3?

  • Taurie Davis changed milestone to %10.3

    changed milestone to %10.3

  • Author Developer

    @tauriedavis everything is working, I'm just clearing the JS part of code + I want to add a couple of specs. So I'm sure I'll finish it soon. 🙂

  • Awesome, thanks @blackst0ne!

  • blackst0ne added 1257 commits

    added 1257 commits

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • Author Developer

    @tauriedavis could you help me here a bit please if you get a chance? 🙂

    I need a feedback about all that spacing/margins/paddings.

    Current view:
    image

    • Resolved by Taurie Davis

      @blackst0ne Of course! I tried to check out your branch to give specific feedback but I just keep seeing "Checking branch availability..." and the button never appears.

      From looking at that screenshot, here are a couple things that pop out:

      • Dropdown section of button should be focused when dropdown is open
      • There is too much spacing between the Branch name input and the Source label. The spacing between these should be 16px
      • Too much spacing between Source input and Create a merge request button. The spacing between these should be 16px.
      • The margin around the inputs/button should be 10px

      I created this spec preview to help see what the spacing should be between each element:

      https://gitlab-org.gitlab.io/gitlab-design/hosted/taurie/ce%2321143-spec-previews/

  • Sean McGivern mentioned in merge request !15176 (closed)

    mentioned in merge request !15176 (closed)

  • blackst0ne added 92 commits

    added 92 commits

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    • ec0b7eb8 - Add active state to dropdown toggle button

    Compare with previous version

  • blackst0ne added 2 commits

    added 2 commits

    Compare with previous version

  • mentioned in issue #40085 (closed)

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne marked the checklist item Has been reviewed by UX as completed

    marked the checklist item Has been reviewed by UX as completed

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne resolved all discussions

    resolved all discussions

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    • 4b29b1e2 - Improve tests and refactored JS

    Compare with previous version

  • mentioned in issue #40320 (closed)

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 1 commit

    added 1 commit

    Compare with previous version

  • blackst0ne added 346 commits

    added 346 commits

    Compare with previous version

  • blackst0ne marked the checklist item Add backend specs as completed

    marked the checklist item Add backend specs as completed

  • blackst0ne added 1 commit

    added 1 commit

    • c80c8c0f - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • Author Developer

    @iamphill @smcgivern I've updated both frontend and backend specs.

    Could you please take the final reviews and, if everything is OK, merge this MR? 🙂

  • assigned to @smcgivern

  • Sean McGivern
  • Sean McGivern resolved all discussions

    resolved all discussions

  • Sean McGivern
  • @blackst0ne LGTM, I think there is more to review on the FE side though.

  • assigned to @iamphill

  • blackst0ne changed title from WIP: Resolve "Customize branch name when using create branch in an issue" to Resolve "Customize branch name when using create branch in an issue"

    changed title from WIP: Resolve "Customize branch name when using create branch in an issue" to Resolve "Customize branch name when using create branch in an issue"

  • Sean McGivern resolved all discussions

    resolved all discussions

  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Thanks @blackst0ne 🎉 just some minor points & then i'll create the EE merge request for this 👍

  • blackst0ne added 1 commit

    added 1 commit

    • eccb132d - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • blackst0ne marked the checklist item Add frontend specs as completed

    marked the checklist item Add frontend specs as completed

  • blackst0ne marked the checklist item Tests added for this feature/bug as completed

    marked the checklist item Tests added for this feature/bug as completed

  • blackst0ne marked the checklist item Has been reviewed by Backend as completed

    marked the checklist item Has been reviewed by Backend as completed

  • blackst0ne added 1 commit

    added 1 commit

    • f46d1e84 - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • assigned to @iamphill

  • blackst0ne added 1 commit

    added 1 commit

    • 92f5390b - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • Author Developer

    There is an EE-to-CE merge conflict in the app/services/merge_requests/build_service.rb file.

    I'm going to handle it when the MR gets merged.

    /cc @smcgivern

  • @blackst0ne thanks, although we typically handle conflicts before merging.

    @iamphill back to you!

  • Tiny suggestion! 👍 It works great though! 😄

  • blackst0ne added 1 commit

    added 1 commit

    • 403f2a8f - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • blackst0ne resolved all discussions

    resolved all discussions

  • blackst0ne added 1 commit

    added 1 commit

    • 5bc32b65 - Add an ability to use a custom branch name on creation from issues

    Compare with previous version

  • Author Developer

    @iamphill I've addressed all the frontend discussions. 👍🏻

    @smcgivern I've refactored the app/services/merge_requests/build_service.rb#assign_title_and_description (split code into separate methods just like it's represented in EE). The EE port is https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3554

    Everything is green now. 🙂

  • assigned to @smcgivern

  • Frontend LGTM 👍

  • Phil Hughes marked the checklist item Has been reviewed by Frontend as completed

    marked the checklist item Has been reviewed by Frontend as completed

  • Sean McGivern approved this merge request

    approved this merge request

  • Thanks @blackst0ne, nice work!

  • Sean McGivern mentioned in commit f3ac2e56

    mentioned in commit f3ac2e56

  • merged

  • Gittaca mentioned in issue #35433 (moved)

    mentioned in issue #35433 (moved)

  • mentioned in issue #41563 (closed)

  • mentioned in issue #41688 (moved)

  • mentioned in issue #41727 (closed)

  • Sean McGivern mentioned in merge request !16356 (merged)

    mentioned in merge request !16356 (merged)

  • mentioned in issue #41938 (closed)

  • mentioned in issue #48025 (closed)

  • Please register or sign in to reply
    Loading