460372 - The confirmation thrown when deleting a custom field, component, version or product must have a red background if the deletion affects bugs

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

All other administrative confirmations displayed when deleting a value have a red background, because data loss may happen. When you delete a custom field, the confirmation has no red background. I barely pay attention to it despite it's a rather important message. It should use the same class as other confirmations.

Comment 1

[Ges Seger]

Attachment: Adds CSS class to warning text

Patch generated using "diff", not cvs.  

After examining Bugzilla's stylesheets, the class "areyoureallysure" seemed to be the one to use here.

Comment 2

[Frédéric Buclin]

Comment on attachment 363147 [details] [diff] [review]
Adds CSS class to warning text

This class doesn't exist.

Comment 3

[Ges Seger]

Attachment: Corrects senior moment on CSS class added previously

Re-examined the directory tree.  The class "throw_error" in skins/standard/global.css has the style settings I remembered (red background in slightly larger font size with extra padding) from the class I thought existed.  Let's go with that.

Comment 4

[Nitish Bezzala]

Attachment: added admin.css and used the class "areyoureallyreallysure"

Comment 5

[Frédéric Buclin]

Comment on attachment 364726 [details] [diff] [review]
Corrects senior moment on CSS class added previously

We should have a separate class for such warning. throw_error displays the text as bold and bigger, which is not necessary here. Also, this class should be used everywhere, for such warnings.

Comment 6

[Frédéric Buclin]

Comment on attachment 377655 [details] [diff] [review]
added admin.css and used the class "areyoureallyreallysure"

Not the right class. Text is red, not the background.

Comment 7

[Nitish Bezzala]

I looked at the confirmations while deleting a classification and deleting a keyword, but they don't have a class with a red background. 
Should I modify the areyoureallyreallysure class to have a red background and black text?

Comment 8

[Frédéric Buclin]

(In reply to comment #7)
> Should I modify the areyoureallyreallysure class to have a red background and
> black text?

No, this class should be left alone. A new class is needed.

Comment 9

[Nitish Bezzala]

Attachment: added class "areyousure" and used it while deleting a custom field

Comment 10

[Frédéric Buclin]

Comment on attachment 379831 [details] [diff] [review]
added class "areyousure" and used it while deleting a custom field

>+p.areyousure {

The classed shouldn't be tied to <p>. It may be used with e.g. <div>.


>+    font-size: 120%;
>+    font-weight: bold;

Both should go away. We are not throwing an error, only a confirmation message.

Comment 11

[Nitish Bezzala]

Attachment: Fixed ".areyousure"

Comment 12

[Frédéric Buclin]

Delaying to 3.8, so that more places can use it. Having it in a single place only for 3.6 would be confusing.

Comment 13

[Frédéric Buclin]

Comment on attachment 408587 [details] [diff] [review]
Fixed ".areyousure"

>Index: skins/standard/admin.css

>+.areyousure {
>+    background-color: red;
>+    color: black;
>+}

This class needs some padding and margin (both set to 1em seems good). Also, we should find a way to not have the whole line to have a red background, but only the part of it containing text (so if the text is short enough to fit in one third of the width of the window, then only one third of the width of the window should have a red background).

Comment 14

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 15

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v1

Repatching it with, similar to "areyoureallyreallysure" css class used for confirmation messages but with some less emphasis on text.

Comment 16

[Frédéric Buclin]

Comment on attachment 673537 [details] [diff] [review]
Patch-v1

>+.areyousure {

Let's name it .confirmation.


>+    color: red;

We want the background in red, not the text itself, see previous comments.

Comment 17

[Sunil Joshi [:sjoshi]]

Attachment: Patch-v2

This patch also takes care of Confirmation messages while deleting Product, version, component and milestone.

For Milestone case, it was already implemented in table, updated the code to use .confirmation class with <p> tag.

Comment 18

[Frédéric Buclin]

Comment on attachment 682516 [details] [diff] [review]
Patch-v2

>=== modified file 'skins/standard/admin.css'

>+    padding: 3ex 3ex 3ex 3ex;

Nit: no need to repeat 3ex four times. Just once is enough.


r=LpSolit

Comment 19

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified skins/standard/admin.css
modified template/en/default/admin/components/confirm-delete.html.tmpl
modified template/en/default/admin/custom_fields/confirm-delete.html.tmpl
modified template/en/default/admin/milestones/confirm-delete.html.tmpl
modified template/en/default/admin/products/confirm-delete.html.tmpl
modified template/en/default/admin/versions/confirm-delete.html.tmpl
Committed revision 8502.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.4/
modified skins/standard/admin.css
modified template/en/default/admin/components/confirm-delete.html.tmpl
modified template/en/default/admin/custom_fields/confirm-delete.html.tmpl
modified template/en/default/admin/milestones/confirm-delete.html.tmpl
modified template/en/default/admin/products/confirm-delete.html.tmpl
modified template/en/default/admin/versions/confirm-delete.html.tmpl
Committed revision 8478.