Fixes: Library - Connection state's position when there are no libraries around #11442 #12948
Conversation
github-actions
bot
added
APP: Learn
There was a problem hiding this comment.
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. |
|
This is SS of the change, I have also added the changes in ordered manner Please look into it. I tried using Kresponsive Element but still i have to add css at breakpoints because of logo there its overlapping with text in some screen sizes. |
|
@LianaHarris360 Please Review the changes so I can do any other change if required |
Build Artifacts
|
There was a problem hiding this comment.
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.
| @@ -13,12 +14,7 @@ | |||
| <h1 :style="{ marginLeft: '-8px' }"> | |||
| {{ injectedtr('otherLibraries') }} | |||
| </h1> | |||
| </KGridItem> | |||
| <KGridItem | |||
There was a problem hiding this comment.
Removing the usage of KGridItem here or the margin-left: -190px; on line 322 might have had unwanted effects on the icon and string shown when searching for other libraries:
I also noticed that when the window is large, the No other libraries around you right now and Showing all available libraries around you descriptions are no longer positioned underneath the Other Libraries section header:
I think any changes you make to the connection state when there are no libraries should also be compared with the other two connection state descriptions to make sure there are no regressions.
| @@ -257,4 +286,67 @@ | |||
| margin-left: 0.75em; | |||
| } | |||
|
|
|||
| @media screen and (max-width: 600px) { | |||
There was a problem hiding this comment.
useKResponsiveWindow is used for styling based on reactive window size information rather than CSS Media queries. For more detailed breakpoint styling, using KGridItem along with useKResponsiveWindow could be useful. There’s some examples of this being done in the Kolibri codebase here, here, and here.
I saw that you mentioned logo overlapping with text on some screen sizes; perhaps this issue could be fixed by using conditional styling?
| v-show="!searchingOtherLibraries && !devicesWithChannelsExist" | ||
| data-test="no-other" | ||
| class="a" |
There was a problem hiding this comment.
CSS class names are most effective when they are descriptive and meaningful, and indicate the purpose of the element. It would be best to avoid using vague or generic names.
Maybe instead of class names a and b, something like no-other-div and no-other-label-div? This will also make the CSS easier to read and helps other devs quickly identify which element it is referring to.
| } | ||
|
|
||
| .disco { | ||
| margin-right: -640px; |
There was a problem hiding this comment.
I noticed more than 10 instances of negative margins being used. While they can be useful at times, they can be difficult to debug and make the CSS harder to read, so we try to avoid them or add a comment explaining its usage. Is there an alternative you can use in place of some of these?
|
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! |
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)