-
Notifications
You must be signed in to change notification settings - Fork 716
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
Updates AppBar styles to be consistent on desktop #11008
Updates AppBar styles to be consistent on desktop #11008
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.
Just a couple of questions.
@@ -331,4 +331,8 @@ | |||
font-size: 14px; | |||
} | |||
|
|||
// .subpage-nav { |
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.
What did this CSS class definition do to deserve this purgatory? Maybe just delete it if it is unneeded?
}, | ||
showSoudNotice() { | ||
return this.isLearnerOnlyImport && (this.isSuperuser || this.isAdmin || this.isCoach); | ||
}, | ||
showAppNavView() { |
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.
Is there anywhere else that this is needed? I am wondering if just having a micro-composable defining this might be good to share between the two components, so that it's always consistent.
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.
The only other place this is used is in AppBar on line 22 https://github.com/learningequality/kolibri/pull/11008/files#diff-2d30fc6599689a247a3fada5ab24f57827027d082c7af44fe001d961a3018553R22. I could make a composable for this if it's worth it? The other conditions on that page around app/desktop display are related but not the same.
…ot, and limit app nav styles to mobile and tablet app
…o files that manage app navigation UI
6825366
to
5a37699
Compare
Build Artifacts
|
Following a conversation with in Slack with Endless/LE and after checking in with @tomiwaoLE - we decided that the desktop navigation experience should be consistent across browser and app, and the new "app navigation" should be only for app + mobile/tablet screen size combination.
Summary
Adjusts the
References
Remediates #11079
Slack conversation
Reviewer guidance
Confirm in app and desktop that the new criteria are met - the only thing that should be different is that on app + desktop, we have the traditional top bar menu tab navigation, not the "app" menu. Everything else should remain the same.
Testing checklist
PR process
Reviewer checklist
yarn
andpip
)