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

Distinguish destructive actions #4374

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Nov 23, 2023

Distinguish destructive actions

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

closes #1815

Description

  • Marks destructive ft-buttons, ft-icon-buttons, and ft-prompt buttons with appropriate red styling (source). This is changed to a purple-ish color when the user has red theming set, and a yellow-ish color when the user has both red and purple theming set. It uses black as opposed to white for the corresponding text color, improving the color contrast from 3.68:1 to 5.7:1. I tried keeping it unaltered, but I was having legitimate problems trying to read the labels. This isn't as good of a color contrast as I wanted (see my preferred coloring in 73690fa), but it's a compromise because it seems like the darker red is not as aesthetically pleasing to some.
  • Changes ft-prompt confirmation text labels to be more explicit (source)
  • Implements icon prop for ft-button to put trash icon on all destructive button actions (note: excluding "Delete Selected" button in Profile Settings that tows the line as a reset button)
  • Makes "Cancel" prompt buttons colorless for the few that were not already colorless
  • Removes one-off show-close prop on ft-prompt in favor of just defaulting the last option to be the close/cancel one

Screenshots

Screenshot_20240424_224320
Screenshot_20240424_220905
Screenshot_20240424_221933
Screenshot_20240424_224343

Testing

  • Change primary or secondary theme color to a red color and confirm purple color for destructive actions
  • Change primary and secondary theme color to any red and purple colors and confirm yellow color for destructive actions
  • Confirm all destructive buttons are marked with destructive coloring
  • Confirm all destructive ft-prompt buttons are marked in destructive color & with more explicit text, and test their functionality
  • Confirm playlist prompt "Cancel" buttons are now colorless

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

One way to make the red color an even better comparative color contrast without changing it would be making the corresponding text color black. This is an even better color contrast of 5.7:1. This would be my most preferred choice, but the only issue is that it's not the existing status quo, so I'm worried that would be some source of conflict. Thoughts?

Screenshot_20240425_063822

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) November 23, 2023 06:01
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 23, 2023
@PikachuEXE
Copy link
Collaborator

PikachuEXE commented Nov 23, 2023

I also think the color is way too dark to represent danger/destructive
I know it's trying to have a different red but it I rather just same the same red by default and choose another color for "red users" (not everyone use red as primary)

Update 1: purple in destructive color to use existing purple

image
image

image
image

.system[data-system-theme*='light'], .light,
.system[data-system-theme*='dark'], .dark,
.black,
.gray,
.dracula,
.catppuccinMocha,
.pastelPink,
.hotPink {
  --primary-input-color: rgba(0, 0, 0, 0.50);

  --destructive-color: #f44336;
  --destructive-text-color: #FFFFFF;
  --destructive-hover-color: #e53935;
  --destructive-active-color: #c62828;
}

.mainRed {
  --primary-color: #f44336;
  --primary-color-hover: #e53935;
  --primary-color-active: #c62828;

  --destructive-color: #9C27B0;
  --destructive-text-color: #FFFFFF;
  --destructive-hover-color: #8E24AA;
  --destructive-active-color: #6A1B9A;
}

@PikachuEXE
Copy link
Collaborator

Also might conflict when 2nd color set to red

Tested quick fix below (never meant to be final)
image
image

.secRed {
  --accent-color: #f44336;
  --accent-color-hover: #e53935;
  --accent-color-active: #c62828;
  --accent-color-light: #ef9a9a;
  --accent-color-visited: #b71c1c;
  --accent-color-opacity1: rgba(244,67,54,0.04);
  --accent-color-opacity2: rgba(244,67,54,0.12);
  --accent-color-opacity3: rgba(244,67,54,0.16);
  --accent-color-opacity4: rgba(244,67,54,0.24);

  --destructive-color: #9C27B0;
  --destructive-text-color: #FFFFFF;
  --destructive-hover-color: #8E24AA;
  --destructive-active-color: #6A1B9A;
}

@kommunarr
Copy link
Collaborator Author

Hi Pika, updated as per your suggestion to use the darker red only when the main or secondary theme is red. Purple is nonstandard for this, so would still prefer to use some gradient of red in any case if possible. But willing to give on that if good reason given and/or others prefer it that way.

Copy link
Member

Choose a reason for hiding this comment

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

This PR introduces a bug:
Open Experimental Settings -> Enable toggle -> Click Cancel -> FT restarts and toggle is enabled in settings when u come back to it after restart

@kommunarr kommunarr force-pushed the feat/mark-destructive-buttons-as-red branch from 7817c30 to 69d155d Compare November 23, 2023 21:42
@efb4f5ff-1298-471a-8973-3d47447115dc

Is setting a password destructive?

@kommunarr
Copy link
Collaborator Author

If it doesn't delete data, or more loosely, force exit of the program, no, that wouldn't be befitting.

@PikachuEXE
Copy link
Collaborator

The dark still look ugly
Not to mention it's not tested on icon button yet (and I just checked there is CSS change but no JS/vue file change)

And yup the dark red really looks ugly AND I don't feel danger from dark red when I use red theme already
image

@PikachuEXE
Copy link
Collaborator

Here is another attempt to change the color to resolve conflict with red color themes (main + sec)

Screenshot 2023-11-24 at 08 24 06
Screenshot 2023-11-24 at 08 24 15
Screenshot 2023-11-24 at 08 24 30
Screenshot 2023-11-24 at 08 24 40

/* region destructive color for red color themes */

.mainRed, .secRed {
  --destructive-color: #9C27B0;
  --destructive-text-color: #FFFFFF;
  --destructive-hover-color: #8E24AA;
  --destructive-active-color: #6A1B9A;
}

/* Deal with theme conflict */
.mainRed.secPurple ,
.mainPurple.secRed {
  --destructive-color: #FF9800;
  --destructive-text-color: #FFFFFF;
  --destructive-hover-color: #FB8C00;
  --destructive-active-color: #EF6C00;
}

/* endregion */

@kommunarr
Copy link
Collaborator Author

kommunarr commented Nov 24, 2023

Not that I'm inherently opposed to it, but what's the design precedent/source/rationale for using purple or orange to represent a destructive action? Just want to make sure we have firm grounding for the decision that we make rather than "vibes" per se (not saying that you are doing this, just clarifying my thought process)

@PikachuEXE
Copy link
Collaborator

I would argue that using color is not enough for representing danger actions (especially for red color theme users)
So first of all dark red is really not effective to convoy the danger message
Using purple/orange is just an attempt while staying inside the scope of changing color

I have no idea for icon button yet but for button with text we can see if any idea from the follow article can be used
https://uxmovement.com/buttons/how-to-design-destructive-actions-that-prevent-data-loss/

@kommunarr
Copy link
Collaborator Author

I guess it's just a rough situation in general that we have a red color we use for buttons, as it leads to these types of situations. I do see how a darker red could potentially be overlooked (as a black button, I suppose? Let me know if I understand that point correctly) for a user with red theming, but I just don't have any reason to believe that a purple or yellow button would be any better, as they aren't conventionally associated or recognized as destructive colors (even worse, I think they draw attention & intrigue without communicating a clear risk).

I do think the prompt label updates should help, and maybe I can look into adding a "Trash" icon to the top of the prompts like that article suggests. If I go that route, I may also have to remove a couple of instances that are toeing the line of counting as destructive (that force a close & restart of the app).

@kommunarr kommunarr added PR: WIP and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 24, 2023
@PikachuEXE
Copy link
Collaborator

I think having red buttons on other color themes are fine, but for red color themes, no idea at all
So going adding icon might even be better

Still no clue what to do with icon buttons (to be used in playlist)

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Nov 25, 2023

Maybe a weird thought but i put it out here:

What about not changing the red color of the buttons when red theming is selected. Yes users will see all red buttons but as fallback we now have a pop up prompt that has a white cancel button and a red yes button so that might be a clear indicator something serious is going on there. So maybe it doesnt even matter because we have that fallback in place.

@PikachuEXE
Copy link
Collaborator

I have no idea until the icons are added
Also it might feel different for red as main theme and red as secondary theme
I will wait for icons to be added first (and I only use red as main theme)

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Apr 20, 2024

Looking back at it, I'm kinda liking the purple destructive for users with red themes, as it least it catches attention (for everybody but the rare user who has both red & purple themes). I'm gonna wait until #3975 is merged before working on this one again, as I want to add trash icons by destructive modals.

Edit: sorry for closing the PR accidentally, am having some keyboard issues.

@kommunarr kommunarr closed this Apr 20, 2024
auto-merge was automatically disabled April 20, 2024 14:58

Pull request was closed

Copy link
Contributor

github-actions bot commented May 1, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 1, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

PikachuEXE
PikachuEXE previously approved these changes May 1, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Based on the previous review I did a sanity check and found this prompt that is shown as destructive but isnt

VirtualBoxVM_vMXhTM1bd2.mp4

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 1, 2024
@FreeTubeBot FreeTubeBot merged commit 19c5966 into FreeTubeApp:development May 2, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 2, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented May 2, 2024

This prompt didnt change is that intentional? Shouldnt the close button be white?

VirtualBoxVM_l8EOtoU32w.mp4

@kommunarr
Copy link
Collaborator Author

I have a PR fixing this and two more minor issues related to this PR I discovered after more testing.

@efb4f5ff-1298-471a-8973-3d47447115dc

Was this button intentionally unchanged? This doesnt seem like a destructive action. Shouldnt this be blue?

FreeTube_GRIpprHp2h.mp4

@kommunarr
Copy link
Collaborator Author

Are you sure you don't have something set up in your theme? I'm not seeing that

@PikachuEXE
Copy link
Collaborator

I saw the same
image

My theme settings
image

@kommunarr
Copy link
Collaborator Author

Your main color theme is red

@efb4f5ff-1298-471a-8973-3d47447115dc

Main point is that all red buttons are now destructive but this isn't a destructive action. So why would we want this to stay red?

@kommunarr
Copy link
Collaborator Author

It's not a destructive action. Check the code. It's because your main theme color is red

@efb4f5ff-1298-471a-8973-3d47447115dc

I know this action is NOT destructive and that it is based on the main color theme.

The thing is it doesnt make sense to keep this on based on the main color theme.

All destructive buttons are based on the main color theme.

This is the only non destructive button that is based on the main color theme.

My suggestion would be to change this to the secondary color theme

@absidue
Copy link
Member

absidue commented May 15, 2024

@efb4f5ff-1298-471a-8973-3d47447115dc The destructive buttons aren't based in the main color, they use red regardless of your theme, unless your main or secondary color is red, then it uses purple and if either of them are purple it uses yellow. So yes they don't fit in with any theme.

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.

Mark destructive buttons as red
5 participants