Resolve "Customize branch name when using create branch in an issue"
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?
-
Changelog entry added, if necessary -
Documentation created/updated -
API support added -
Tests added for this feature/bug - Review
-
Has been reviewed by UX -
Has been reviewed by Frontend -
Has been reviewed by Backend -
Has been reviewed by Database
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Squashed related commits together
What are the relevant issue numbers?
Closes #21143 (closed)
Merge request reports
Activity
mentioned in issue #21143 (closed)
marked the checklist item Changelog entry added, if necessary as completed
added 1 commit
- b8ccc37f - Add ability to create MR with custom branch name
- Resolved by blackst0ne
Our current logic of generating an MR description is based on a branch name.
If a user changes a branch name from a default to a custom one, the current implementation of the
assign_title_and_description
method will not append theCloses iid
text to the MR description.What should I do with it?
I personally prefer always pass an iid of an issue and always append theCloses xxx
string, but there might be some reasons why we don't do that right now.🙂
- Resolved by blackst0ne
The same question as above about the
Resolves ...
MR title which is generated depending on an target issue.On a custom branch name the BuildService will not generate the
Resolves...
title.We can pass the title, or do something else.
Edited by blackst0ne
added 1 commit
- 49e0bc2a - Add ability to create a branch with custom source
added 1 commit
- 49e0bc2a - Add ability to create a branch with custom source
- Resolved by blackst0ne
I want to add an ability to check if a entered custom branch name is already taken. I can use the public API endpoint
HEAD /projects/:id/repository/branches/:branch
or add a one more method to the
issues_controller
next to thecan_create_branch
: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/controllers/projects/issues_controller.rb#L182-192What is the preferred way?
🙂 I would use a single source of data (API endpoints).
added 1 commit
- 555a6d36 - Use default source branch if the ref input if empty
added 1 commit
- d7d8f158 - Remove descriptions from the options in the dropdown
- Resolved by blackst0ne
@tauriedavis I deleted the descriptions from the dropdown items. But I don't like how it now looks like. What do you think? Can/should we improve it somehow?
added 1 commit
- 6ae07521 - Add green status for the first time opened dropdown
added 1 commit
-
e48b12b9 - Fix both the
Closes
reference and theResolve
title
-
e48b12b9 - Fix both the
- Resolved by blackst0ne
@kushalpandya @lbennett @ClemMakesApps
Guys, I got stuck with javascript here (dropdowns/droplab).
🙁
Could you help me a bit, please?Here is the latest state of the
Create merge request
button.A few questions.
🙂 - I can't get it, why the whole dropdown get closed when I chose a source branch. Adding IGNORE_CLASS to any element does not help me. Is there any class conflicts?
- If I chose a branch name from the dropdown, the button selected element gets wrong. As I can see, this happens because the
drop_down.js
takes the closestli
element and marks it as selected which is wrong. It was fine when the dropdown had only two variants without any inputs.
So what's the proper way to make things workable and not overcomplicated?
🙂
@blackst0ne Just wanted to give a heads up I will be on vacation next week. Please reach out to @sarrahvesselov if you need some UX guidance here. She can either help or direct you to another UXer who can help/review!
🌟 @tauriedavis thank you for the information!
👍🏻 @sarrahvesselov might also be unavailable because of the hurricane, just fyi
changed milestone to %10.1
assigned to @ClemMakesApps
Thanks @ClemMakesApps! I am back if needed for anything
😄 assigned to @lbennett
added 1786 commits
-
7cbd70d1...20295b3d - 1762 commits from branch
master
- 21006a59 - Add a changelog entry
- 9e5920c6 - Add input fields to the dropdown
- 4bc862bd - Fix autoclosing dropdown
- 30b62cbf - Init JS functions
- 71a04575 - Add ability to create MR with custom branch name
- bfd621a3 - Create new branch from the source branch
- 39c79a30 - Add ability to create a branch with custom source
- 0541ac1d - Add availability messages
- 338ec2bd - Use default branch name if input is empty
- eeaf14de - Use default source branch if the ref input if empty
- d70ad583 - Remove descriptions from the options in the dropdown
- bb002698 - Add support for Enter key
- b907e0e5 - Add support for the ESC key
- deec9193 - Add green status for the first time opened dropdown
- 1a05fa6e - Fix rubocop offenses
-
a5a85740 - Fix both the
Closes
reference and theResolve
title - bf83c1e4 - Add branch existence check
- 4470c151 - Fix UI of items
- a59b27d2 - Fix description of api helpers
- 6ab502fa - Fix eslint offenses
- ba647712 - Fix rubocop offense
- 52701fec - Trim whitespaces
- ac548f53 - Add source branch dropdown
- b18cc233 - Get rid of not-selectable class
Toggle commit list-
7cbd70d1...20295b3d - 1762 commits from branch
- Resolved by blackst0ne
Working on this MR, I got some tricky issues related to dropdown within another dropdown.
Both @kushalpandya and @ClemMakesApps said me that using nested dropdowns are not a good idea from the frontend perspective.Can we do here something else from the UX perspective?
Here is how it looks like at the moment on my computer:
I can see two options here (at least this is what comes to my mind first):
- Do not use any nested dropdown for the source branch. Let a user just type it (the default value would be a project's default branch).
- Load all available branches and use a simple list. But I don't like this as it will fit the performance issues even if we are going to use ajax loading.
What do you think about this?
🙂 ps: this MR is almost ready, just needs to solve this problem.
added UX label
assigned to @blackst0ne
added 477 commits
-
b18cc233...05d8e87d - 451 commits from branch
master
- e395d9c2 - Add a changelog entry
- 380ce0af - Add input fields to the dropdown
- 9c70d1de - Fix autoclosing dropdown
- a746c08b - Init JS functions
- f45c8908 - Add ability to create MR with custom branch name
- 86ac128f - Create new branch from the source branch
- a5e64890 - Add ability to create a branch with custom source
- 9744d45b - Add availability messages
- 6df6ad7c - Use default branch name if input is empty
- 00f70dfe - Use default source branch if the ref input if empty
- 154d9091 - Remove descriptions from the options in the dropdown
- f2ebf3d4 - Add support for Enter key
- b351c79f - Add support for the ESC key
- 4d642a4e - Add green status for the first time opened dropdown
- 459ecd24 - Fix rubocop offenses
-
6ba0df10 - Fix both the
Closes
reference and theResolve
title - d80e8e60 - Add branch existence check
- d0003250 - Fix UI of items
- 9ba525e4 - Fix description of api helpers
- 58087983 - Fix eslint offenses
- eb692268 - Fix rubocop offense
- 45c5092c - Trim whitespaces
- 0c7a9569 - Add source branch dropdown
- 4841be8e - Get rid of not-selectable class
- 01d9478f - Add init support for source searching
- ce55986a - Refactor js
Toggle commit list-
b18cc233...05d8e87d - 451 commits from branch
- Resolved by blackst0ne
I got some inconsistency while working on this MR.
If you want to create a new merge request, you are not able to pick a tag or commit as a source. See: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/new
But in the mockups we do support
branch
,tag
, andcommit
as a source of a new branch in theCreate merge request
button.What should I do here?
Should I make things consistent and add only branch support or should I still add everything as a source?
- Resolved by blackst0ne
@tauriedavis in the original issue there are two mockup styles:
- When the button is green.
- When the button is gray.
The questions are:
- Which one is it going to be? Gray or green?
- Should the button be clickable as it is right now? Or should only the right arrow down button be clickable?
added 1 commit
- 8e4985a3 - Add an ability to use a custom branch name on creation from issues
added 1 commit
- 909b0e8a - Add an ability to use a custom branch name on creation from issues
The work is done. Only specs are left. But I'd like to pass the first round of UX/frontend/backend reviews just to avoid rewriting specs if anything should be changed.
@tauriedavis could you review the UX part, please? I updated the description (added a screen record of how this feature works).
@smcgivern could you review the backend part, please?
🙂 @timzallmann @jschatz1 could you ask someone from your teams to take a review, please? I don't know who I should mention here from the frontend teams.
🙁 marked the checklist item Squashed related commits together as completed
added 1 commit
- 25779fbe - Add an ability to use a custom branch name on creation from issues
- Resolved by blackst0ne
- Resolved by Sean McGivern
- Resolved by Sean McGivern
@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.
assigned to @timzallmann
- Resolved by blackst0ne
@blackst0ne Very cool, @iamphill can you review please?
assigned to @iamphill
added workflowin review label
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by Phil Hughes
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Dropdown is too close to the button
- According to the designs there shouldn't be a divider here
-
The refs endpoint doesn't return all refs. So for
gitlab-ce
I can typemaster
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 typemaster
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.
-
master
inbranch name
&master
in source input doesn't do anything. It just fails silently.
assigned to @blackst0ne
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.
- Resolved by blackst0ne
changed milestone to %10.2
mentioned in issue #21552 (closed)
added 2 commits
@smcgivern I changed the backend code. Could you review again, please?
🙂 assigned to @smcgivern
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by Sean McGivern
Thanks @blackst0ne, the backend LGTM so far!
assigned to @blackst0ne
mentioned in issue #39293 (closed)
- Resolved by blackst0neEdited by blackst0ne
- Resolved by blackst0neEdited by blackst0ne
- Resolved by blackst0ne
The refs endpoint doesn't return all refs. So for
gitlab-ce
I can typemaster
in the source branch text box & it says it is not available.The
branch name
text box always says its available. I typemaster
which I know isn't available & it never changed. I guess is because of point 3.Edited by blackst0ne
- 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
- Resolved by blackst0ne
There is a point on the 2
Create..
rows that is clickable & closes the dropdown.
- Resolved by blackst0ne
Shouldn't these 2 rows have some descriptive text?
- Resolved by blackst0ne
- Resolved by blackst0ne
master
inbranch name
&master
in source input doesn't do anything. It just fails silently.
- 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.
added 1541 commits
-
d8a2346f...e1122c9f - 1540 commits from branch
master
- adf70c27 - Add an ability to use a custom branch name on creation from issues
-
d8a2346f...e1122c9f - 1540 commits from branch
@iamphill I've changed the code. Could you review the frontend part, please?
🙂 assigned to @iamphill
- Resolved by Phil Hughes
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
The spacing in the dropdown looks really weird, a lot more than it probably should be. cc. @tauriedavis
I get this flicker when moving my mouse over the different options:
Button looks like it doesn't match any of our other buttons & also has a flicker.
assigned to @blackst0ne
Regarding spacing, it should match exactly our other dropdowns. See the milestone dropdown as an example:
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:
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?
changed milestone to %10.3
@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!
added 1257 commits
-
599d5532...0c3877a4 - 1255 commits from branch
master
- 4d0dd527 - Merge remote-tracking branch 'upstream/master' into…
- a33dfa80 - Use debounce
-
599d5532...0c3877a4 - 1255 commits from branch
@tauriedavis could you help me here a bit please if you get a chance?
🙂 I need a feedback about all that spacing/margins/paddings.
- 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 theSource
label. The spacing between these should be 16px - Too much spacing between
Source
input andCreate 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/
mentioned in merge request !15176 (closed)
added 92 commits
-
bf7127bd...c6a48f3f - 89 commits from branch
master
- e97005cd - Merge remote-tracking branch 'upstream/master' into…
- 0c306a0c - Refactor UI
- 28b62a18 - Remove redundant variable
Toggle commit list-
bf7127bd...c6a48f3f - 89 commits from branch
mentioned in issue #40085 (closed)
mentioned in issue #40320 (closed)
added 346 commits
-
757e2dc0...acae8ddb - 345 commits from branch
master
- 767c6465 - Add an ability to use a custom branch name on creation from issues
-
757e2dc0...acae8ddb - 345 commits from branch
added 1 commit
- c80c8c0f - Add an ability to use a custom branch name on creation from issues
@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
- Resolved by Sean McGivern
- Resolved by Sean McGivern
@blackst0ne LGTM, I think there is more to review on the FE side though.
assigned to @iamphill
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
- Resolved by blackst0ne
Thanks @blackst0ne
🎉 just some minor points & then i'll create the EE merge request for this👍 assigned to @blackst0ne
added 1 commit
- eccb132d - Add an ability to use a custom branch name on creation from issues
added 1 commit
- f46d1e84 - Add an ability to use a custom branch name on creation from issues
assigned to @iamphill
added 1 commit
- 92f5390b - Add an ability to use a custom branch name on creation from issues
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!
assigned to @blackst0ne
added 1 commit
- 403f2a8f - Add an ability to use a custom branch name on creation from issues
added 1 commit
- 5bc32b65 - Add an ability to use a custom branch name on creation from issues
@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 inEE
). The EE port is https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/3554Everything is green now.
🙂 assigned to @smcgivern
Thanks @blackst0ne, nice work!
mentioned in commit f3ac2e56
mentioned in issue #35433 (moved)
mentioned in issue #41563 (closed)
mentioned in issue #41688 (moved)
removed workflowin review label
mentioned in issue #41727 (closed)
mentioned in merge request !16356 (merged)
mentioned in issue #41938 (closed)
mentioned in issue #48025 (closed)
added devopsplan label