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

Feat/3637/add apply button #3795

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

BenediktMehl
Copy link
Collaborator

@BenediktMehl BenediktMehl commented Oct 22, 2024

Add an apply button when selecting maps

Closes: #3637

Description

Added a button below the map selection.
Removing and changing the selection state of maps is only applied, if user clicks the apply button.

Definition of Done

A PR is only ready for merge once all the following acceptance criteria are fulfilled:

  • Changes have been manually tested
  • All TODOs related to this PR have been closed
  • There are automated tests for newly written code and bug fixes
  • All bugs discovered while working on this PR have been submitted as issues (if not already an open issue)
  • Documentation (GH-pages, analysis/visualization READMEs, parser READMEs, --help, etc.) has been updated (almost always necessary except for bug fixes)
  • CHANGELOG.md has been updated

Screenshots or gifs

Screenshot 2024-10-22 at 13 15 21 Screenshot 2024-10-22 at 13 15 13

Copy link
Collaborator

@IhsenBouallegue IhsenBouallegue left a comment

Choose a reason for hiding this comment

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

The code looks good, I just have a couple of suggestions that might make this more intuitive.

  1. Gray out and disable the Apply button when there are no changes, enable it when there is. That makes it clear when something changed and you have to apply.
  2. The text on hover of the delete button is Remove map even when the button is restoring it.
  3. Instead of a + icon we can use an undo, maybe it's clearer that it undoes the "delete".

Copy link

sonarcloud bot commented Oct 30, 2024

Copy link

sonarcloud bot commented Oct 30, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an apply button when selecting maps
2 participants