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

Adds feature to discard all the changes on the map #10045

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Asif-Sheriff
Copy link
Contributor

Description

This PR addresses the issue #10033 which concerns the need for a discard button to discard all the changes on the map. Previously this was done by reloading the map and then discarding all the changes but this PR will create a discard button to dump all the changes without having to reload the page, also unlike the reload method this discard action can be undone.

Changes made

Presently this PR has 3 commits

Commit1: creates the discard feature.
Commit2: merges changes made into the original repo.
Commit3: fixes lint issues in the code.

Here's the rundown of changes made in Commit 1:

  1. Creates a discard() function in history.js which sets the current state of the map to the original state.
  2. Created a discard.js file in /tools to render the discard button and write the logic for the same.
  3. Added UI label in helper.js
  4. Added the discard button to top_toolbar.js

Proof of concept

Screencast.from.12-12-23.10.52.08.PM.IST.webm

Final thoughts

I understand that this PR makes some major changes to iD therefore I would request the admins to take a look and this and if there are any changes to be made feel free to let me know as I am more than willing to work further on this issue.

@dypa
Copy link

dypa commented Jan 6, 2024

The demo looks amazing! Сan you add improvements?

  • the "discard" button has replaced the save button location, this may confuse users - you need to move the "discard" button near the "undo" button
  • add a confirmation window
  • check will the translation mechanism work for other languages?

@Asif-Sheriff
Copy link
Contributor Author

@dypa
Sure Ill work on it

@Asif-Sheriff
Copy link
Contributor Author

@dypa
Is this better?
Screencast from 08-01-24 10:42:25 AM IST.webm

@tyrasd tyrasd added usability An issue with ease-of-use or design considering Not Actionable - still considering if this is something we want labels Jan 17, 2024
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be useful in some niche cases. However, I'm not convinced a button in the main edit toolbar is needed for this. I would have put it into the "Map Data" panel, perhaps as in this mockup:

Alternatively, there could be a dropdown/long-click option of the undo button to "Undo everything" (with slightly different semantics: instead of creating a new state which could be undone to get one's edits back in a single click of the Undo button, this would correspond to undo all changes which could be redone one by one using the Redo button).

@tyrasd
Copy link
Member

tyrasd commented Jan 17, 2024

PS: If changes can be undone, there is really no need to add a confirmation dialog. I'd remove it.

PSS: the dist directory should never be manually changed, as it is recreated automatically by the build scripts. New translatable strings should be added to the data/core.yaml file (read more).

@Asif-Sheriff
Copy link
Contributor Author

@tyrasd
Hey I liked the long press functionality for the undo button and here's the implementation

Screencast.from.21-01-24.03.38.15.PM.IST.webm

Like you suggested, long pressing the undo button will discard all the changes and set the state of the map to the way it was initially also the user can redo the changes one by one if they wish.

If this is alright then I will commit the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
considering Not Actionable - still considering if this is something we want usability An issue with ease-of-use or design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants