att / rcloud

Collaborative data analysis and visualization
http://rcloud.social
MIT License
431 stars 142 forks source link

Bugfix/issue 2601 #2605

Closed jameesy closed 6 years ago

jameesy commented 6 years ago

Fixed Issue of "Enter" not progressing the Merge dialogue section.

gordonwoodhull commented 6 years ago

Thanks @JABedford! The changes look good.

Only change I will ask is please don't use tabs in this repo. I think four spaces is the standard indent for JS in this project, although some places is 2 and that is fine too.

It causes a lot of trouble when some people are using tabs and others are using spaces.

Please push a reformat of merger_view.js and I'll merge this to develop soon.

I've assigned you a bunch of other issues which should be straightforward but just ask here or in email if you have any questions.

jameesy commented 6 years ago

Sorry for that! I have put it back to the same state as the other merger files which is two spaces for continuity.

gordonwoodhull commented 6 years ago

Hmm, actually, that's not it. I'm not talking about one line, I'm talking about the entire file.

It looks like in e3c95632732f96216 you (or your text editor :) converted the entire file from spaces to tabs. If you look at the "Files changed" here in GitHub you can see it won't show the diffs by default because the entire file is changed (although you can add ?w=1 to ignore whitespace).

I'd like to get this back to spaces and then squash the PR so that this big change is removed (makes it hard to track changes).

I can do it myself but I'd also like to get the process correct - could you please try again? Thanks!

jameesy commented 6 years ago

Many apologies Gordon. Seemingly this was an issue with my text editor automatically reformatting everything!

I have gone back and cleaned everything up a bit.

gordonwoodhull commented 6 years ago

Yes, editors can get overzealous like that. There's a nice clean diff! Thank you.

gordonwoodhull commented 6 years ago

Fixed a minor typo, renamed the button to just "No Changes", and merged to develop.

Thanks @JABedford!