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

Update badge delegate #5138

Merged
merged 7 commits into from
Jan 14, 2025
Merged

Update badge delegate #5138

merged 7 commits into from
Jan 14, 2025

Conversation

l-olson1214
Copy link
Collaborator

Phabricator:
https://phabricator.wikimedia.org/T382932

Notes

Test Steps

  1. Check on a fresh install the notifications, make sure they go away for logged out users upon clicking

Screenshots/Videos

@l-olson1214 l-olson1214 requested review from a team and mazevedofs and removed request for a team January 10, 2025 16:10
@l-olson1214 l-olson1214 changed the base branch from main to 7.6.3 January 10, 2025 16:38
Copy link
Collaborator

@mazevedofs mazevedofs left a comment

Choose a reason for hiding this comment

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

It's not clearing the badge state for me.
Other than just calling this upon clicking, we have logic in place here to verify if the user has seen the first slide.
Perhaps in this situation, you need to create a separate check for logged out users that clears this state after just clicking the item in menu.
We still need the current logic in place to logged-in users, so a different check would work better in this case.

edit: You don't need another delegate method, but work on YiRDataController whether you should clear the badge immediately or still check if the user saw the first slide

Copy link
Collaborator

@mazevedofs mazevedofs left a comment

Choose a reason for hiding this comment

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

looks good!

@tonisevener tonisevener merged commit 9163ddb into 7.6.3 Jan 14, 2025
4 checks passed
@tonisevener tonisevener deleted the T382932 branch January 14, 2025 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants