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

Drag and drop of semicombos #7024

Merged
merged 1 commit into from Jan 9, 2020
Merged

Drag and drop of semicombos #7024

merged 1 commit into from Jan 9, 2020

Conversation

@TAQ2
Copy link
Contributor

TAQ2 commented Nov 6, 2019

#5728

Drag and drop working fine on full-width tags i.e. Destination but slightly different for the other inputs e.g. Destination Road Numbers. Please can I get some opinions on the code and the functionality

Questions:

  1. Am I supposed to have added tests?
  2. Am I supposed to have added documentation? It would make sense to have it somewhere when the i button is clicked
  3. Is this the correct place to put this code? Its similar functionality to the drag and drop for relations. But seems a bit premature to abstract it out.
  4. I haven't looked into testing mobile - is this something that iD supports?
  5. I am trying to merge to master - not sure if this is correct as I've branched from 2.x . Is master version 3? If so I can't test because master is broken see below
@TAQ2 TAQ2 changed the base branch from 2.x to master Nov 6, 2019
@TAQ2

This comment has been minimized.

Copy link
Contributor Author

TAQ2 commented Nov 6, 2019

Parse error at dist/iD.js:41858,7
	class CountryCoder {
	      ^
ERROR: Unexpected token: name «CountryCoder», expected: punc «;»
    at JS_Parse_Error.get (eval at <anonymous> (/home/travis/build/openstreetmap/iD/node_modules/uglify-js/tools/node.js:18:1),

I have not touched any variable named CountryCoder. Seems like even greenkeeper builds are failing on this error

@quincylvania mind helping me out :)

@quincylvania

This comment has been minimized.

Copy link
Collaborator

quincylvania commented Nov 7, 2019

@TAQ2 Thanks for working on this! I'll have to go through the code later but it seems like a great start.

Am I supposed to have added tests?

Not necessarily, we don't have tests for most stuff in iD right now, particularly in the UI.

Am I supposed to have added documentation? It would make sense to have it somewhere when the i button is clicked

Nope, I think it's fine to let people discover this feature on their own.

Is this the correct place to put this code? Its similar functionality to the drag and drop for relations. But seems a bit premature to abstract it out.

Yes, seems fine. No need to abstract this.

I haven't looked into testing mobile - is this something that iD supports?

Mobile support is a nice-to-have in iD at the moment but not critical. We'll do a full push toward mobile support sometime in the future.

I am trying to merge to master - not sure if this is correct as I've branched from 2.x . Is master version 3? If so I can't test because master is broken see below

master is v3. 2.x is the more "stable" branch. There's no reason this has to wait for v3 so I'd recommend targeting 2.x.

Seems like even greenkeeper builds are failing on this error

Yes, this is because of an error unrelated to your work. This sometimes happens on the development branches. See #7009.

@TAQ2

This comment has been minimized.

Copy link
Contributor Author

TAQ2 commented Nov 25, 2019

@quincylvania thanks for answering those for me. What is the status of this? Did you want me to move it to 2.x branch first? I assumed that you wanted to look at the code first perhaps?

@quincylvania

This comment has been minimized.

Copy link
Collaborator

quincylvania commented Dec 18, 2019

@TAQ2 Hi, apologies for letting this go stale! I haven't reviewed your code in detail but the structure looks fine. Could you resubmit targeting the up-to-date 2.x branch? Then we can get it merged.

@TAQ2 TAQ2 force-pushed the TAQ2:patch-1 branch from 9b6c7fb to 7d579a6 Jan 5, 2020
@TAQ2

This comment has been minimized.

Copy link
Contributor Author

TAQ2 commented Jan 5, 2020

Ok done, I'm not sure how a few of you how a few of your commits landed on here. Please advise as I don't know how to get rid of them

@quincylvania

This comment has been minimized.

Copy link
Collaborator

quincylvania commented Jan 6, 2020

Ok done, I'm not sure how a few of you how a few of your commits landed on here. Please advise as I don't know how to get rid of them

Hmm I don't know either! Looks like you're still targeting the master branch… you might try closing this PR and creating a new branch off of 2.x re-adding your changes.

@SilentSpike

This comment has been minimized.

Copy link
Collaborator

SilentSpike commented Jan 6, 2020

Those commits should disappear if you edit the PR on github (to right of the title) and target it to 2.x

It's because they're not in master

@quincylvania quincylvania changed the base branch from master to 2.x Jan 6, 2020
@quincylvania

This comment has been minimized.

Copy link
Collaborator

quincylvania commented Jan 6, 2020

Those commits should disappear if you edit the PR on github (to right of the title) and target it to 2.x

It's because they're not in master

@SilentSpike Thanks for the tip!

@TAQ2 TAQ2 force-pushed the TAQ2:patch-1 branch from 7d579a6 to ba00334 Jan 6, 2020
@TAQ2

This comment has been minimized.

Copy link
Contributor Author

TAQ2 commented Jan 6, 2020

Thanks for the help

@quincylvania quincylvania added the field label Jan 6, 2020
@quincylvania quincylvania self-assigned this Jan 9, 2020
@quincylvania quincylvania reopened this Jan 9, 2020
@quincylvania quincylvania merged commit e2e0ad3 into openstreetmap:2.x Jan 9, 2020
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@quincylvania

This comment has been minimized.

Copy link
Collaborator

quincylvania commented Jan 9, 2020

@TAQ2 I tested this out and it looks pretty good! I merged it as-is but I'm going to make a few tweaks on my end. Thanks again for your work! 🎉

@quincylvania quincylvania added this to the 2.17.1 milestone Jan 9, 2020
quincylvania added a commit that referenced this pull request Jan 9, 2020
Don't try reordering multiCombo fields
Fix offset behavior for full-width semicombos
Use grab/grabbing cursor style
Keep dragged chip above others
quincylvania added a commit that referenced this pull request Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.