-
Notifications
You must be signed in to change notification settings - Fork 793
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
LessonResourceSelection: Add margin to bottom controls when isAppContext & touch device #13215
base: develop
Are you sure you want to change the base?
LessonResourceSelection: Add margin to bottom controls when isAppContext & touch device #13215
Conversation
…ext && isTouchDevice In Quiz this is not necessary because QuizCreation is in its own immersive page when the user is selecting resources, so the BottomNavigationBar is not present. However, in Lessons, the base Lesson page is not an immersive page, so the app-mode-specific menu is still visible, causing the side panel's bottom controls to be hidden behind the menu. Rather than fiddling w/ z-indexes or trying to conditionally hide that menu, this just puts the Save & Finish button in a visible place
Build Artifacts
|
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.
Code-wise looks good to me! Just a little question
<div | ||
class="bottom-nav-container" | ||
:style="{ | ||
marginBottom: isAppContextAndTouchDevice ? '56px' : '0px', |
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.
Defenitely non blocking, and speaking from ignorance, could this condition to render the botton menu change, for any reason, in the future? If so, I think remember to update also this specific component in this specific context will be a little hard. Just wondering if should we have something like a showAppNavView
variable inside the useUser
perhaps? To control the exact conditions to render the app nav, and replace them in the AppBarPage too, and ensuring that if they change in the future, all places that depends on it will be updated accordingly.
If this condition is something we dont foresee can change, and this is over-engineering 😅, i think its okay then.
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.
Having it available in useUser would be nice to have - I think it'd be useful in a few others places too. I think it might be a good follow-up tech-debt issue to consolidate the conditionals that ask "is app context && is touch device" into useUser, then update wherever we're using it to get that info from useUser.
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.
Sounds good! <3
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.
I think I'd probably prefer it to be somewhere other than useUser
(even if it depends on useUser) as it's not about the user per se - and yes, we had a similar concern when this was first implemented, so doing something to consolidate it would be helpful.
Summary
Ensures Save & Finish is visible on Android by adding padding when isAppContext and isTouchDevice (aka, when the BottomNavigationBar is present).
From the commit message:
References
Fixes #13172
Reviewer guidance
In the app context, can you see Save & Finish in lesson resource selection? Can you view all resources in the cards list during:
Additionally, does it look correct in not app context?