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

Add requesting review #4223

Merged
merged 32 commits into from Aug 17, 2017

Conversation

Projects
None yet
7 participants
@kepta
Collaborator

kepta commented Aug 8, 2017

This adds review request.

@pnorman

This comment has been minimized.

Show comment
Hide comment
@pnorman

pnorman Aug 8, 2017

Contributor

It'd be good to sort the tagging out with the community, not on the iD issue tracker.

Contributor

pnorman commented Aug 8, 2017

It'd be good to sort the tagging out with the community, not on the iD issue tracker.

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 8, 2017

Member

It'd be good to sort the tagging out with the community, not on the iD issue tracker.

Why? My feeling has always been that:

  1. The iD issue tracker is part of the community. It's open, and anybody who wants can follow what we do or comment or suggest changes.
  2. Changeset tags are really more decided by the editor software than by anybody else.
  3. We can use any tags we like.

I am really happy to change the tag, if anybody cares. It doesn't have to be review_requested=yes.

Member

bhousel commented Aug 8, 2017

It'd be good to sort the tagging out with the community, not on the iD issue tracker.

Why? My feeling has always been that:

  1. The iD issue tracker is part of the community. It's open, and anybody who wants can follow what we do or comment or suggest changes.
  2. Changeset tags are really more decided by the editor software than by anybody else.
  3. We can use any tags we like.

I am really happy to change the tag, if anybody cares. It doesn't have to be review_requested=yes.

bhousel added some commits Aug 10, 2017

Remove the link to the osm wiki changeset page
(doesn't really add anything and not written for newbies)
WIP: refactor uiCommit into sections, introduce uiChangesetEditor
uiCommit is getting kind of big as we add more to the commit pane.

I'm going to split it up and put the field rendering code into a separate
module, similar to how uiEntityEditor embeds uiPresetEditor for the fields.

This allows us to add a few more fields that users can set on their changesets
(like hashtags, source), and even hide them under a "Add field" dropdown.
@iandees

This comment has been minimized.

Show comment
Hide comment
@iandees

iandees Aug 11, 2017

Collaborator

I realize I could check this out and look, but what are y'all thinking about for prompting the user to ask for review? A popup? A checkbox near the changeset save button?

Collaborator

iandees commented Aug 11, 2017

I realize I could check this out and look, but what are y'all thinking about for prompting the user to ask for review? A popup? A checkbox near the changeset save button?

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 11, 2017

Member

This request review field is great and I could merge it right now, but instead I'm going to spend some time to refactor the commit pane a little bit - it is like 500 lines of code and getting difficult to manage as we add things to it.

Managing a bunch of fields and the tags that they change is something that uiEntityEditor and uiPresetEditor already do well, so I'm going to use that as a model to split uiCommit into smaller modules.

@iandees, here's a screenshot showing current placement of the checkbox, along with some dummy fields that I stuck at the top. The dummy fields are being rendered by the new uiChangesetEditor component.

screenshot 2017-08-11 17 29 02

Member

bhousel commented Aug 11, 2017

This request review field is great and I could merge it right now, but instead I'm going to spend some time to refactor the commit pane a little bit - it is like 500 lines of code and getting difficult to manage as we add things to it.

Managing a bunch of fields and the tags that they change is something that uiEntityEditor and uiPresetEditor already do well, so I'm going to use that as a model to split uiCommit into smaller modules.

@iandees, here's a screenshot showing current placement of the checkbox, along with some dummy fields that I stuck at the top. The dummy fields are being rendered by the new uiChangesetEditor component.

screenshot 2017-08-11 17 29 02

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 14, 2017

Member

Refactoring the commit pane by using the new uiChangesetEditor module is working nicely. It now creates real uiField objects and supports a "more fields" section. We can let users tag a source now. Here are some things that I need to put back:

  • dropdown for previous changesets
  • placeholder texts
  • google warning
  • enable save button when comment exists
  • double check the warnings and summary sections of uiCommit

and some scope creep..

  • #2834 - add hashtags
  • #4230 - esc should cancel save mode
  • #3968 - few more changeset tags.. first_edit, completed_walkthrough, others?
Member

bhousel commented Aug 14, 2017

Refactoring the commit pane by using the new uiChangesetEditor module is working nicely. It now creates real uiField objects and supports a "more fields" section. We can let users tag a source now. Here are some things that I need to put back:

  • dropdown for previous changesets
  • placeholder texts
  • google warning
  • enable save button when comment exists
  • double check the warnings and summary sections of uiCommit

and some scope creep..

  • #2834 - add hashtags
  • #4230 - esc should cancel save mode
  • #3968 - few more changeset tags.. first_edit, completed_walkthrough, others?
@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 14, 2017

Member

Looks like this currently

save mode

Member

bhousel commented Aug 14, 2017

Looks like this currently

save mode

@iandees

This comment has been minimized.

Show comment
Hide comment
@iandees

iandees Aug 14, 2017

Collaborator

It would be great to think of a way to call out that "request review" checkbox, especially for new folks. Can it pulsate or be a different color or have an arrow with a friendly globe suggesting they give it a try?

Collaborator

iandees commented Aug 14, 2017

It would be great to think of a way to call out that "request review" checkbox, especially for new folks. Can it pulsate or be a different color or have an arrow with a friendly globe suggesting they give it a try?

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 14, 2017

Member

It would be great to think of a way to call out that "request review" checkbox, especially for new folks. Can it pulsate or be a different color or have an arrow with a friendly globe suggesting they give it a try?

I have thoughts on this request review checkbox and how it might be perceived by different kinds of users.

Not everybody wants feedback on their edits. For example, millennials may be more likely to want the feedback, and genx'ers may be more likely to not want the feedback. Majority users may be more comfortable with public edits, and minorities may not feel comfortable having their edits under additional scrutiny.

I think this feature could be very helpful and welcome to a lot of people - especially new users - but I don't want to put assumptions into the software, and so that's why we ask the user whether they want the review or not.

Also at this point we don't know how many people will check the box, or who will step up to do the reviews. The answer is some combination of - OSMCha, whodidit, bots, rss feeds, dedicated validators, paid mapping teams, and volunteers.

I thought about just checking the box if it's a user's first edit, or having the preference be "sticky", but I don't want to promise something the OSM community can't deliver. So for now, let's leave the box unchecked, and "opt-in" to review. We can change this later and encourage more users to check the box, if after a few months we decide that the reviews are working well and helpful.

Member

bhousel commented Aug 14, 2017

It would be great to think of a way to call out that "request review" checkbox, especially for new folks. Can it pulsate or be a different color or have an arrow with a friendly globe suggesting they give it a try?

I have thoughts on this request review checkbox and how it might be perceived by different kinds of users.

Not everybody wants feedback on their edits. For example, millennials may be more likely to want the feedback, and genx'ers may be more likely to not want the feedback. Majority users may be more comfortable with public edits, and minorities may not feel comfortable having their edits under additional scrutiny.

I think this feature could be very helpful and welcome to a lot of people - especially new users - but I don't want to put assumptions into the software, and so that's why we ask the user whether they want the review or not.

Also at this point we don't know how many people will check the box, or who will step up to do the reviews. The answer is some combination of - OSMCha, whodidit, bots, rss feeds, dedicated validators, paid mapping teams, and volunteers.

I thought about just checking the box if it's a user's first edit, or having the preference be "sticky", but I don't want to promise something the OSM community can't deliver. So for now, let's leave the box unchecked, and "opt-in" to review. We can change this later and encourage more users to check the box, if after a few months we decide that the reviews are working well and helpful.

@mvexel

This comment has been minimized.

Show comment
Hide comment
@mvexel

mvexel Aug 14, 2017

Contributor

Agree with @bhousel -- until we know how many requests will actually be honored it's probably better to not call it out too much. If people don't want to review or use discouraging words to do so it would just serve to turn more people off than the other way around.

Contributor

mvexel commented Aug 14, 2017

Agree with @bhousel -- until we know how many requests will actually be honored it's probably better to not call it out too much. If people don't want to review or use discouraging words to do so it would just serve to turn more people off than the other way around.

@talllguy

This comment has been minimized.

Show comment
Hide comment
@talllguy

talllguy Aug 14, 2017

Here's an idea for frequently used sources as checks, streamlining taginfo a bit
image

talllguy commented Aug 14, 2017

Here's an idea for frequently used sources as checks, streamlining taginfo a bit
image

bhousel added some commits Aug 14, 2017

Restore "good changeset comments" link in tag-reference section
Also includes a bunch of css cleanups for the tag-reference section
to fix style for readonly raw-tag-editor rows and adjust margin widths
@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 15, 2017

Member

Hey @talllguy, one benefit of the recent refactoring work is that it makes it super easy to just reuse fields like the ones that iD already has other places. We don't have a field that's an array-of-checkboxes, but we do have a multiselect field that turns the values into a semicolon separated list.

What do you think of this?:

sources

Member

bhousel commented Aug 15, 2017

Hey @talllguy, one benefit of the recent refactoring work is that it makes it super easy to just reuse fields like the ones that iD already has other places. We don't have a field that's an array-of-checkboxes, but we do have a multiselect field that turns the values into a semicolon separated list.

What do you think of this?:

sources

@talllguy

This comment has been minimized.

Show comment
Hide comment
@talllguy

talllguy Aug 15, 2017

@bhousel I like to see this fields reused this way. This implementation looks cool! I'd use this.

Next steps would be using an algorithm to analyze the changeset comment and other metadata to suggest source tags. ;)

talllguy commented Aug 15, 2017

@bhousel I like to see this fields reused this way. This implementation looks cool! I'd use this.

Next steps would be using an algorithm to analyze the changeset comment and other metadata to suggest source tags. ;)

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 16, 2017

Member

FYI just pushed a working implementation of hashtags (see #2834).
This is just another semicolon-delimited multiselect field, and I added some extra code to detect hashtags embedded in the comment.

The idea here is to give mapathons, task managers, QA tools, a special field to store their hashtags, instead of (ab)using the comment field, which some people don't like. Hashtags will also be settable through the API just like comments currently are.

hashtags

Member

bhousel commented Aug 16, 2017

FYI just pushed a working implementation of hashtags (see #2834).
This is just another semicolon-delimited multiselect field, and I added some extra code to detect hashtags embedded in the comment.

The idea here is to give mapathons, task managers, QA tools, a special field to store their hashtags, instead of (ab)using the comment field, which some people don't like. Hashtags will also be settable through the API just like comments currently are.

hashtags

bhousel added some commits Aug 16, 2017

Store changesets_count in a changeset tag
(it will say "0" for someone making their first edit)
Write changeset tags for new mappers to indicate walkthrough progress
These tags all start with `ideditor:`
(closes #3968)

```
ideditor:walkthrough_completed=yes
ideditor:walkthrough_progress=welcome;navigation;point;area;line;building;startEditing
ideditor:walkthrough_started=yes
```

@bhousel bhousel merged commit 0ea043b into master Aug 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bhousel bhousel deleted the request_review branch Aug 17, 2017

@bhousel

This comment has been minimized.

Show comment
Hide comment
@bhousel

bhousel Aug 17, 2017

Member

Just merged this - thanks for your patience!
Hope these new features will make it easier for other projects to analyze changesets.

Member

bhousel commented Aug 17, 2017

Just merged this - thanks for your patience!
Hope these new features will make it easier for other projects to analyze changesets.

simon04 pushed a commit to openstreetmap/josm that referenced this pull request Sep 4, 2017

floscher pushed a commit to floscher/josm that referenced this pull request Sep 4, 2017

floscher pushed a commit to floscher/josm that referenced this pull request Sep 14, 2017

@matkoniecz

This comment has been minimized.

Show comment
Hide comment
@matkoniecz

matkoniecz Jan 9, 2018

Thanks, this feature helps to distinguish clueless initial edits from complex things that may be intentional.

It is also good reminder that somebody may be just starting and to avoid complaining too much.

matching JOSM ticket: https://josm.openstreetmap.de/ticket/15754#ticket

matkoniecz commented on 25d8a8a Jan 9, 2018

Thanks, this feature helps to distinguish clueless initial edits from complex things that may be intentional.

It is also good reminder that somebody may be just starting and to avoid complaining too much.

matching JOSM ticket: https://josm.openstreetmap.de/ticket/15754#ticket

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