-
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
Fixes: Library - Connection state's position when there are no libraries around #11442 #12948
base: develop
Are you sure you want to change the base?
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.
Hi @yashhash2, thank you. High-level feedback from me:
- Would you upload before/after screenshots to the pull request description?
- Generally, we don't use CSS media queries. Instead we utilize useKResponsiveWindow that is integrated with our breakpoint system. You could read the docs and study another places in the codebase. Then I'd ask you to remove the CSS media queries in favor of
useKResponsiveWindow
. - I see many duplicate styles. Would you place styles common for all breakpoints outside particular breakpoint styles so they are applied to all breakpoints? Then from each breakpoint style only override what is needed for that particular breakpoint. That should simplify code significantly.
I will invite my colleague @LianaHarris360 for a full review as soon as this is resolved. Liana, would you please have a look then? I only did a brief skim myself. This is a new attempt at #11442 which you reviewed once here #12405. |
Thank you @MisRob for feedback i will try to incorporate as many points as possible. |
@LianaHarris360 Please Review the changes so I can do any other change if required |
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.
Thanks so much for your work on this @yashhash2, this is a tricky issue to fix. There were a few things I pointed out in my review. Please be aware that using CSS media queries is not a recommended practice in the Kolibri codebase. Although it is a straightforward fix, it can lead to challenges with maintenance, and there is a well-established alternative that we use consistently throughout the codebase.
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
Thank you for your feedback @LianaHarris360 I will try to do all the changes you mentioned above ASAP |
There's no hurry :) Since Learning Equality will be closing for the holidays soon, I might not be able to review more changes until after the new year. Thank you for your efforts! |
Hey @LianaHarris360 I tried using KGridItem instead of CSS media queries and here are some issues in which I need clarifications
Key Concern: Despite testing multiple span sizes within the layout configuration (in layout4), this transition anomaly persists. Guidance on resolving this is essential.
Example: Wi-Fi icon misplacement below 400px: Questions: |
Hi @yashhash2 It would be helpful if I could see the changes you've made in the code; however, I do see that the “Other Libraries” label and the sync-status div are positioned in two KGridItem components next to each other. However, with the way KGridItem operates, because both KGridItem components have a span of 6 set for the Perhaps if the span for the KGridItems were increased to numbers greater than 6 in As for the position of the Wi-Fi icon on small screens, it looks like that placement is done manually in the code. I'm not exactly sure why it's set up this way, but I'll investigate to find out the reason. However, since this was done on purpose and is not a result of your changes, you don't need to worry about it! |
48eaf03
to
2757f3c
Compare
Hi @LianaHarris360, I've committed the code for you to review. If you'd like me to revert the span of the other-Libraries grid to its original size, I can make that adjustment as well. Please note that the breaking point is still the primary concern, and the current layout places the K-grid items below one another due to their span sizes exceeding 6 and 4, respectively. Let me know your thoughts or if you have additional feedback. Best regards, |
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 for making the suggested changes and updating the grid span sizes, we are making great progress! I noted a few things that could improve the spacing of the connection status test.
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/views/LibraryPage/OtherLibraries.vue
Outdated
Show resolved
Hide resolved
Thank You Lana Harris for the Review I will do the recommended changes ASAP : ) |
Hi @LianaHarris360, I wanted to let you know that we have successfully resolved the issues we discussed earlier. The following changes were made: Removed the no-other-Libraries and Other-libraries sections. Please review the changes at your convenience. Below is the final result for your reference. Best regards, |
</span> | ||
</div> | ||
</KGridItem> | ||
</KGridItem> |
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.
It seems that this KGridItem
is placed within another one, but the otherLibraries text should be within its own KGridItem
, separate from the grid where the connection status is located, like this:
<KGridItem>
<h1> otherLibraries text </h1>
</KGridItem>
<KGridItem>
<div class="sync-status">
</div>
</KGridItem>
</style> | ||
.connection-status{ | ||
margin-bottom: 30px; | ||
margin-top: -15px; |
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 negative margin-top
here combined with the negative margin-bottom
on line 7 makes the connection status text too close to the text above it and is inconsistent with the spacing of the text below it, removing the negative margins and perhaps making the margin-bottom
in this class smaller should even it out.
Hi @LianaHarris360 Sorry for dragging this issue to this far. I wasnt sure about the exact spacing which is required so here is the final preview of the current spacing. |
Summary
So I created media queries for specific screen sizes to display the connection state position correctly. applied Padding and margins wherever necessary and created 3 DIVS a, b and disco because when state was offline position was changed again,by using these DIVS I was able to fix issue for both offline and online state.
References
#11442 (comment)