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

Factor new issue and edit MR forms. #7678

Merged
merged 1 commit into from Sep 4, 2014

Conversation

4 participants
@cirosantilli
Contributor

cirosantilli commented Sep 3, 2014

There was a lot of code duplication here.

There is also a third copy which has diverged more and which we should DRY out as well: the new MR form. It probably has the best visual style of the three and should be the one to copy. Opened an issue for that at: https://github.com/gitlabhq/gitlabhq/issues/7679 so we won't forget.

The issue edit and new were already factored.

Minor visible changes:

  • "Issue is parsed with GFM" message changed to: "Parsed with GFM". Shorter, DRYer, and it is obvious that we are talking about the description that corresponds to the textarea above.

I cannot see any other behavior / UI changes.

Non-visible changes:

  • title input element:
    • remove rows attribute. Does nothing on input type="text".
    • remove class "pad" from the merge request form. Not used anywhere in the app.

TODO in future merge requests: DRY those forms even further on the points they diverged more:

  • errors
  • labels
  • submit

Found using https://github.com/UncleGene/flay-haml

@TeatroIO

This comment has been minimized.

TeatroIO commented Sep 3, 2014

I've prepared a stage. Click to open.

= f.label :title, class: 'control-label' do
%strong= 'Title *'
.col-sm-10
-# TODO dry out the 255

This comment has been minimized.

@Razer6

Razer6 Sep 3, 2014

Member

@cirosantilli Is this PR still WIP?

This comment has been minimized.

@cirosantilli

cirosantilli Sep 4, 2014

Contributor

@Razer6 ah sorry, I was considering to factor this out as it duplicates model information, but in the end decided not too. We should be using https://github.com/plataformatec/simple_form, it can factor this, and much more.

This comment has been minimized.

@Razer6

Razer6 Sep 4, 2014

Member

@cirosantilli Sounds interesting. @randx What are thoughts a bout it? I ❤️ refactoring because it removes so much duplicated code!

This comment has been minimized.

@dzaporozhets

dzaporozhets Sep 4, 2014

Member

I think this refactoring makes sense

@cirosantilli cirosantilli force-pushed the cirosantilli:factor-new-issue-edit-mr branch from 08e6f9b to 788033a Sep 4, 2014

@dzaporozhets

This comment has been minimized.

Member

dzaporozhets commented Sep 4, 2014

@cirosantilli it is ready for merge right?

@cirosantilli

This comment has been minimized.

Contributor

cirosantilli commented Sep 4, 2014

@randx Yes, we factor the 255 on another PR.

dzaporozhets added a commit that referenced this pull request Sep 4, 2014

Merge pull request #7678 from cirosantilli/factor-new-issue-edit-mr
Factor new issue and edit MR forms.

@dzaporozhets dzaporozhets merged commit bd00233 into gitlabhq:master Sep 4, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

@cirosantilli cirosantilli deleted the cirosantilli:factor-new-issue-edit-mr branch Sep 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment