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

fix: globally fix text color of buttons in alerts #3323

Merged
merged 2 commits into from
Jan 6, 2025

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Dec 19, 2024

Fixes text color of all primary buttons within alerts.

See Kong/kongponents@84a4e9f

I previously "fixed" this in #3317 just for our upgrade button, thinking this was the only instance

This PR undoes that ^ PR and fixes it globally instead, which will fix the issue for all instances whether I remember about them or not.

Before

Screenshot 2024-12-19 at 11 22 19

After

Screenshot 2024-12-19 at 11 08 31

@johncowen johncowen requested a review from a team as a code owner December 19, 2024 11:21
@johncowen johncowen requested review from schogges and removed request for a team December 19, 2024 11:21
Copy link

netlify bot commented Dec 19, 2024

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit 6504917
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/677bbbcf59dfe700084bdd03
😎 Deploy Preview https://deploy-preview-3323--kuma-gui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@schogges schogges left a comment

Choose a reason for hiding this comment

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

Nice catch 👍 also happy to see further adoption of variant 🙂

@johncowen
Copy link
Contributor Author

FYI I had/still have another branch which takes a completely different approach and makes it impossible for these things to happen at all (this PR only fixes primary buttons within a KAlert).

It's an interesting branch/piece of work, so might put it up anyway to share. Would be good to chat with someone about it whether we wanted to use it or not (I'm not totally convinced yet myself).

@johncowen
Copy link
Contributor Author

johncowen commented Jan 6, 2025

Ah, I found out how to access existence of @events a couple of weeks back and it was something I wanted to use here but couldn't figure out how at the time (events are also present on attrs prefixed with on it seems). I added a commit here also so that just by adding a @dismiss handler it makes the alert :dismissable.

Will leave this hanging a little while longer here in case, but will merge later today if I hear no more.

@schogges
Copy link
Contributor

schogges commented Jan 6, 2025

Ah, I found out how to access existence of @events a couple of weeks back and it was something I wanted to use here but couldn't figure out how at the time (events are also present on attrs prefixed with on is seems). I added a commit here also so that just by adding a @dismiss handler it makes the alert :dismissable.

Will leave this hanging a little while longer here in case, but will merge later today if I hear no more.

Ah, yeah that is good to know 👍

@johncowen johncowen merged commit adf5cfa into kumahq:master Jan 6, 2025
16 checks passed
johncowen added a commit that referenced this pull request Jan 24, 2025
### current master (I couldn't manage to get a grab with rollover, but
it underlines):

![Screenshot 2025-01-24 at 14 23
55](https://github.com/user-attachments/assets/1174fabb-e5e7-45ee-afec-df4ec3769fde)

### With #3460 applied (the mouse
is rolled over the link but the pointer isn't in the grab):

![Screenshot 2025-01-24 at 14 18
00](https://github.com/user-attachments/assets/8b096d74-d29c-4ce6-beff-3285e3e73933)

### With this PR applied (the mouse is rolled over the link but the
pointer isn't in the grab):

![Screenshot 2025-01-24 at 14 17
08](https://github.com/user-attachments/assets/3a65998b-59fb-4f4b-9697-095c3072063d)

Introduced in Kong/kongponents#2585

I've mentioned a few times I kinda have a different approach to solve
this sort of stuff completely, I might put up a draft PR soon.

Related to:

- Kong/kongponents#2559
- #3323
- #3317

---------

Signed-off-by: John Cowen <[email protected]>
johncowen added a commit that referenced this pull request Jan 24, 2025
it underlines):

![Screenshot 2025-01-24 at 14 23
55](https://github.com/user-attachments/assets/1174fabb-e5e7-45ee-afec-df4ec3769fde)

is rolled over the link but the pointer isn't in the grab):

![Screenshot 2025-01-24 at 14 18
00](https://github.com/user-attachments/assets/8b096d74-d29c-4ce6-beff-3285e3e73933)

pointer isn't in the grab):

![Screenshot 2025-01-24 at 14 17
08](https://github.com/user-attachments/assets/3a65998b-59fb-4f4b-9697-095c3072063d)

Introduced in Kong/kongponents#2585

I've mentioned a few times I kinda have a different approach to solve
this sort of stuff completely, I might put up a draft PR soon.

Related to:

- Kong/kongponents#2559
- #3323
- #3317

---------

Signed-off-by: John Cowen <[email protected]>
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.

2 participants