-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
🐛 monitor middleware - fix ignore custom settings #2024
🐛 monitor middleware - fix ignore custom settings #2024
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some test cases
would be nice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are not fixing the problem. The else statement never runs if the default Title
and Refresh
are used which is the original issue
To be honest I don't really understand what the point of this judgment is, it seems to me to be unnecessary. |
No judging, just wanted to point the issue with the URL's. The original commit fixed the issue for the "refresh" field. Looks better now! 👍🏻 |
@ReneWerner87 @efectn @gaby Please check out the new changes when you have time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks! 🚀
Please provide enough information so that others can review your pull request:
Fix monitor middleware ignores custom settings if default title is used.
Explain the details for making this change. What existing problem does the pull request solve?
close #2019