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

Replace javascript:; links with buttons. #7793

Merged
merged 1 commit into from Sep 23, 2014

Conversation

5 participants
@cirosantilli
Copy link
Contributor

commented Sep 19, 2014

Behavior improvements:

  • middle click on the button does not open a javascript:; tab
  • on hover browser does not show the link destination javascript:;

Appearance changes: none.

Also more semantic: links exist to redirect your browser somewhere else. This explains the better behavior.

Works because button.btn type="button" and a.btn are styled almost exactly the same in Bootstrap.

Just for reference, the elements that are affected are the "Reply" button and the "+" on diff comments:

screenshot from 2014-09-19 15 24 22 gitlab javascript button to

@TeatroIO

This comment has been minimized.

Copy link

commented Sep 19, 2014

I've prepared a stage. Click to open.

@cirosantilli cirosantilli changed the title [factor] Replace javascript:; links with buttons. Replace javascript:; links with buttons. Sep 19, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:replace-javascript branch 3 times, most recently from db1fe05 to bc97259 Sep 19, 2014

@@ -206,4 +206,9 @@ def line_code
def line_code_2
sample_compare.changes.last[:line_code]
end

def click_diff_line(data=nil)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Sep 19, 2014

Surrounding space missing in default value assignment.

@cirosantilli cirosantilli force-pushed the cirosantilli:replace-javascript branch from bc97259 to cb7633d Sep 19, 2014

@Razer6

This comment has been minimized.

Copy link
Member

commented Sep 19, 2014

@randx Looks good!

@Razer6 Razer6 added this to the 7.4 milestone Sep 19, 2014

link_to "", "javascript:;", class: "add-diff-note js-add-diff-note-button",
data: data, title: "Add a comment to this line"
button_tag '', class: 'btn add-diff-note js-add-diff-note-button',
data: data, title: 'Add a comment to this line'

This comment has been minimized.

Copy link
@dzaporozhets

dzaporozhets Sep 22, 2014

Member

@cirosantilli why this spaces?

This comment has been minimized.

Copy link
@cirosantilli

cirosantilli Sep 22, 2014

Author Contributor

@randx I think Hound requires it saying that we must align the hash keys. I will try without to make sure.

@cirosantilli cirosantilli force-pushed the cirosantilli:replace-javascript branch 2 times, most recently from c64ded1 to c4fdf75 Sep 22, 2014

@cirosantilli cirosantilli force-pushed the cirosantilli:replace-javascript branch from c4fdf75 to 778afb6 Sep 22, 2014

@cirosantilli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2014

Failure appears unrelated and not reproduced locally.

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

Merge pull request #7793 from cirosantilli/replace-javascript
Replace javascript:; links with buttons.

@dzaporozhets dzaporozhets merged commit 6e328d5 into gitlabhq:master Sep 23, 2014

1 check failed

default The build failed on Semaphore.
Details

@cirosantilli cirosantilli deleted the cirosantilli:replace-javascript branch Sep 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.