-
Notifications
You must be signed in to change notification settings - Fork 256
[Remove Vuetify from Studio] Side panel (container only) in Content Library #5555
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
base: unstable
Are you sure you want to change the base?
[Remove Vuetify from Studio] Side panel (container only) in Content Library #5555
Conversation
|
Also, regarding the wrapped string translation issue: since the strings have been moved from the old CatalogFilters component to the new component, they are not updating with the selected language. I wanted to confirm whether I should update the translation entries in the language JSON files by replacing the old component keys with the new component keys. Just let me know what you prefer, and I’ll go ahead with it! :) |
3d59da9 to
5a9c0e0
Compare
5a9c0e0 to
17d0758
Compare
AlexVelezLl
left a comment
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.
Thanks a lot @Prashant-thakur77! We are on a good track! The only alarming thing I found is that the general layout could be handled differently to improve code maintainability; everything else is mostly nitpicks. If you have any questions or run into any problems with the proper layout, whether with flex layouts or without vh, let us know, and we can figure it out together!
| <!-- Main content area that includes CatalogFilterBar and the list --> | ||
| <div | ||
| class="main-content-area" | ||
| :class="{ 'with-sidebar': !isMobile }" | ||
| > | ||
| <CatalogFilterBar /> | ||
| <slot></slot> | ||
| </div> |
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 we can avoid using CatalogFilters as a wrapper component that is responsible for rendering the entire page container. It seems the primary motivation is to set the content area width to width: calc(100% - 335px), but this imposes a very specific, rigid width, which is harder to maintain in the future if any spec changes.
To make this layout more flexible, we can use a parent component for the side panel and the main content with display: flex and flex-direction: row, and set the side panel to a width of 300px, leaving the remaining width for the main content. This way, if in the future we want to change the side panel width, we can override the 300px width, making this layout even flexible enough to support a resizable side panel that takes advantage of the flex machinery.
| <aside | ||
| v-if="!isMobile" | ||
| class="filter-panel-desktop" | ||
| :class="{ 'filter-panel-rtl': isRTL }" |
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.
Seems like there isn't a css class called filter-panel-rtl
| v-if="!isMobile" | ||
| class="filter-panel-desktop" |
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.
In general, We don't usually make differentiations between "mobile vs desktop", and use instead wording like "windowIsSmall" because a Mobile device could render a "windowIsLarge" layout if we rotate it and see the content in landscape mode, and a desktop device could render a "windowIsSmall" layout if the window is shrinked or the user display is splitted displaying two or more apps.
So, it'd be great if we could call all these properties in terms of windowIsSmall/windowIsMedium/windowIsLarge naming that are the ones exposed by the useKResponsiveWindow composable.
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.
In this aside node, we should also set these styles:
- Background color: $themeTokens.surface
- Border: $themeTokens.fineLine
| <SidePanelModal | ||
| v-if="isMobile && showMobilePanel" | ||
| alignment="left" | ||
| fullscreen |
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.
Seems like the SidePanelModal does not have a fullscreen prop
| const { windowIsSmall } = useKResponsiveWindow(); | ||
| return { | ||
| isMobile: windowIsSmall, |
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.
idem, let's just return the windowIsSmall property as is.
| <template> | ||
|
|
||
| <div class="filter-panel-content"> | ||
| <!-- Close button for mobile modal --> |
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.
As a general comment - let's try to remove unnecessary comments. As a specific comment - don't fully get this comment about the close button for mobile modal 😅, in theory, CatalogFilterPanelContent should know nothing about the container where it is being rendered.
| <!-- Language --> | ||
| <LanguageFilter | ||
| v-model="languages" | ||
| :menu-props="menuProps" |
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 know this wasn't introduced by you 😅. But could you please remove this menu-props prop that is not relevant for the LanguageFilter component? Thanks!
| } | ||
| .panel-footer { | ||
| padding: 16px; |
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.
Could we set a horizontal padding of 24px to match the original styles? thanks!
| <KImg | ||
| :src="require('shared/images/le-logo.svg')" | ||
| altText="Learning Equality logo" | ||
| :aspectRatio="'3:2'" | ||
| scaleType="contain" | ||
| :appearanceOverrides="{ | ||
| width: '90px', | ||
| height: '60px', | ||
| marginBottom: '8px', | ||
| marginRight: '8px', | ||
| }" | ||
| /> |
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.
We can update this KImg to:
<KImg
isDecorative
:src="require('shared/images/le-logo.svg')"
backgroundColor="transparent"
scaleType="fitXY"
:style="{
width: '90px',
height: '60px'
}"
/>Changes:
- This image is decorative, i.e. this does not add any information to the page, so we can use the
isDecorativeprop instead of adding analtText. - The KImg documentation shows an example with the scaleType set to
fitXY, there we see that the:styleprop is preferred to set the width and height of the image. - The default background color is a light gray color, so there was a light gray background, and we need to set it to transparent for this.
| .filter-panel-content { | ||
| display: flex; | ||
| flex-direction: column; | ||
| height: calc(100vh - 96px); |
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.
Idem, let's avoid using the vh unit.
|
@AlexVelezLl thanks for your review,I will start working on your comments :) |
FIxes #5528
This PR replaces the Vuetify VNavigationDrawer component in the Content Library side panel with a custom implementation using the Kolibri Design System. The changes include:
For small screens: Using the existing SidePanelModal component to display panel content in a full-screen modal (improving on the previous partial-width behavior)
For medium/large screens: Displaying the panel content as a non-modal page section without using SidePanelModal
Created a reusable inner panel component that works for both modal and non-modal layouts
(Specific tests are yet written for this component,needs clearification)
Preserved all existing filtering functionality and accessibility features
Ensured RTL/LTR compliance and proper mobile responsiveness
Screencast.From.2025-11-17.01-41-20.webm
References
Sub-issue of #5060.
Reviewer guidance
How to get there
Login as [email protected] with password a
Go to Channels > Content Library
Guidance
How to run Kolibri
Find detailed guidance with many code examples in KDS documentation
Read #5060 for more useful references