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

Changed the color of delete button in dashboard from red to grey #4307

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

iamabhshk
Copy link
Contributor

@iamabhshk iamabhshk commented Jan 2, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Changed the color of the delete button in dashboard from red to grey as per the requirement. Just replaced class name from 'btn-danger' to 'btn-secondary' to achieve the changes.
@louislam

Type of change

  • User interface (UI)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • My changes generates no new warnings

Screenshots

Screenshot 2024-01-03 at 12 14 36

@iamabhshk iamabhshk marked this pull request as ready for review January 2, 2024 18:46
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

As noted by nelson in #4290 (comment) or by me in #4290 (comment):
Changing this to be gray does not solve what we want to convey.
Please either

  • hide it in an overflow menu
  • or make this button into a secondary button in style only, not color (prefered by me, idk about @chakflying / @louislam )

Please also include a screenshot of the change you did.

@CommanderStorm CommanderStorm marked this pull request as draft January 2, 2024 20:20
@Zaid-maker
Copy link
Contributor

What's the point of changing it to grey in the first place? Red means danger and it is correct color for delete

@iamabhshk
Copy link
Contributor Author

@CommanderStorm, is this how you wanted?
Screenshot 2024-01-03 130254

@Zandor300
Copy link
Contributor

A suggestion:

btn btn-normal text-danger

Screenshot 2024-01-03 at 12 14 36

@Zaid-maker
Copy link
Contributor

Zaid-maker commented Jan 3, 2024

A suggestion:

btn btn-normal text-danger

yes this looks good

@CommanderStorm CommanderStorm marked this pull request as ready for review January 3, 2024 16:24
@louislam louislam merged commit 23e8088 into louislam:master Jan 3, 2024
17 checks passed
@iamabhshk
Copy link
Contributor Author

Thanks for the opportunity team :)

@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Feb 17, 2024
@ZachFabin
Copy link

@CommanderStorm Why did we remove the red? "red to grey as per the requirement" - I must be missing something; what requirement?

Thanks for your time.

@CommanderStorm
Copy link
Collaborator

@ZachFabin Please see #4290 why we did the change.

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.

6 participants