-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add upgrade motivation banner #7768
Conversation
); | ||
|
||
const getShouldBannerBeShown = () => { | ||
if (!isVersionOutdated) return false; |
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.
(obviously this can be set to true during testing, dont forget to remove)
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.
Cool stuff 😃 During testing, I noticed two things:
- I think the banner should only be shown when logged in. Showing this to external visitors doesn't make much sense, in my opinion.
- webknossos.org should be clickable probably (should open in a new tab)
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.
@philippotto thanks for the feedback! actually it was quite easy to ensure the right nav bar height while switching the theme. I also worked on the case that both a maintenance and an upgrade banner could be shown. I solved it so that only the maintenance banner is shown in that case. Once that is gone, the navbar height is broken (like in your last picture) though. I left it like that because the upgrade banner can be clicked away, because it is a rare case anyway and because fixing it would require letting maintenance and upgrade banner know about each other, which would need some refactoring. Let me know if you disagree and think that I should look into it further :) |
… allow theme change with banner; address review
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.
awesome :)
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)