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

Notify on mention #4538

Merged
merged 9 commits into from
Feb 5, 2025
Merged

Notify on mention #4538

merged 9 commits into from
Feb 5, 2025

Conversation

nicodh
Copy link
Member

@nicodh nicodh commented Jan 23, 2025

Depends on #4589

@nicodh nicodh marked this pull request as draft January 23, 2025 10:26
@nicodh nicodh marked this pull request as ready for review January 29, 2025 13:17
@nicodh nicodh marked this pull request as draft January 29, 2025 13:18
@nicodh nicodh marked this pull request as ready for review January 31, 2025 14:17
@nicodh nicodh requested a review from WofWca January 31, 2025 14:18
@nicodh nicodh removed the new-core involves or requires an core update label Feb 1, 2025
- disable dependendant settings
- switch order All profiles => Current profile
- mark "disabled" switches with opacity: 0.4
@nicodh nicodh force-pushed the notify-on-mention branch from 009daf1 to ec212d3 Compare February 1, 2025 12:02
@WofWca
Copy link
Collaborator

WofWca commented Feb 1, 2025

This closes #4461, right?

@nicodh nicodh requested a review from WofWca February 1, 2025 17:37
@nicodh nicodh requested a review from WofWca February 2, 2025 21:30
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

I tested (almost?) all the cases to see if they comply with the flowchart, and it seems they do.

I'm still not super happy with how many backend calls this introduced, so I hope we'll have all this stuff handled in core in the future. And this setting is disabled by default anyway.

The settings reorder makes sense to me. An input should not affect whether the inputs above it are disabled.
The "disabled" style change also seems good.

Please take a look at the last code suggestion, and let's merge!

@nicodh nicodh merged commit 9e75108 into main Feb 5, 2025
19 checks passed
@nicodh nicodh deleted the notify-on-mention branch February 5, 2025 21:11
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.

3 participants