Skip to content

Normalize confirm remove#5429

Merged
lippserd merged 1 commit intoIcinga:mainfrom
martialblog:normalize-confirm-remove
Mar 26, 2026
Merged

Normalize confirm remove#5429
lippserd merged 1 commit intoIcinga:mainfrom
martialblog:normalize-confirm-remove

Conversation

@martialblog
Copy link
Copy Markdown
Member

@martialblog martialblog commented Oct 2, 2025

This PR normalized the use of the phrase "Confirm Removal" in forms that confirm removal.

It also normalizes the use of the "cancel" icon.

refs #5428

@cla-bot cla-bot bot added the cla/signed label Oct 2, 2025
@martialblog
Copy link
Copy Markdown
Member Author

phpcs fails not due to my code changes. Seems to be the same issue in other PRs Class name "Zend_View_Helper_Util" is not in PascalCase format

@lippserd
Copy link
Copy Markdown
Member

@martialblog Please rebase.

@lippserd lippserd added this to the 2.13 milestone Mar 24, 2026
@martialblog martialblog force-pushed the normalize-confirm-remove branch from 26f476e to 74f9ac2 Compare March 24, 2026 13:18
@martialblog
Copy link
Copy Markdown
Member Author

@lippserd Branch is rebased.

@lippserd lippserd requested a review from jrauh01 March 25, 2026 12:34
Copy link
Copy Markdown
Contributor

@jrauh01 jrauh01 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR - overall this looks good! The adjustment of the cancel icon is consistent.

Two things to address:

  • Role removal button alignment: The confirmation button is currently right-aligned. It should be left-aligned to stay consistent with the other confirmation buttons.
  • Dashboard pane removal: The dashlet removal was updated, but the removal of whole dashboard panes wasn't. That'll need to be covered as well.

@jrauh01
Copy link
Copy Markdown
Contributor

jrauh01 commented Mar 26, 2026

  • Role removal button alignment: The confirmation button is currently right-aligned. It should be left-aligned to stay consistent with the other confirmation buttons.

Did you overlook this one, or are you on it?

@martialblog
Copy link
Copy Markdown
Member Author

@jrauh01 Not 100% sure if this is the correct way to do it cdc20f5 The RoleForm seems to be different than the other forms.

@jrauh01
Copy link
Copy Markdown
Contributor

jrauh01 commented Mar 26, 2026

Yes, unfortunately the forms are all implemented slightly different. But The UserForm does set the .icinga-controls class as well, so I think your solution fits well with the existing code.

@jrauh01
Copy link
Copy Markdown
Contributor

jrauh01 commented Mar 26, 2026

@martialblog I just noticed I overlooked, that the button to remove user groups is right-aligned as well. Your fix won't apply cleanly in this case, and I'm not quite sure if it has to be left-aligned, because of the form description. What do you think about that?

@martialblog
Copy link
Copy Markdown
Member Author

martialblog commented Mar 26, 2026

I'm not a certified UX expert so honestly I can't say. We could also probably change the form description to be an HTML element outside the form and align the button. For me personally it's OK as is and since the description relates to the button it probably best to keep them together e.g. for screenreaders.

@jrauh01
Copy link
Copy Markdown
Contributor

jrauh01 commented Mar 26, 2026

For me it's fine to keep as is as well.

Copy link
Copy Markdown
Contributor

@jrauh01 jrauh01 left a comment

Choose a reason for hiding this comment

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

LGTM!

- Normalize use of removal icons
- Fix tabs in remove pane controller and remove form title
- Set Role removal button left alignment
@martialblog martialblog force-pushed the normalize-confirm-remove branch 2 times, most recently from cdc20f5 to ccedda8 Compare March 26, 2026 14:52
@lippserd lippserd merged commit 9774f11 into Icinga:main Mar 26, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants