Issue#5123 : Migrated the offline alert bar in Settings from Vuetify to Kolibri Design System. #5162
Conversation
|
Thank you @AadarshM07, I will review soon. |
There was a problem hiding this comment.
Very nice work @AadarshM07! I successfully tested on multiple screen sizes, on both RTL and LTR, and with screen reader too. Thanks for following the guidance, much appreciated.
Leaving few notes, let me know what you think.
| icon="disconnected" | ||
| class="mx-3" | ||
| /> | ||
| <span class="notranslate">{{ $tr('offlineText') }}</span> |
There was a problem hiding this comment.
notranslate needs to be removed here - its only for texts that shouldn't be translated. Here we are translating.
There was a problem hiding this comment.
Sure, thanks for clarifying — I will remove it.
| v-if="offline && !libraryMode" | ||
| class="studio-offline-alert" | ||
| :style="computedstyle" | ||
| data-test="studio-offline-alert" |
There was a problem hiding this comment.
We typically use data-test for unit testing and when there is no other way to find an element we need to test. I don't see it being used from anywhere so it could be removed. Alternatively, would you be able to cover the component with few basic unit tests?
There was a problem hiding this comment.
Sure, I noticed that OfflineText.vue already has unit tests that cover similar offline detection logic here. I can take that as a reference and write a separate test suite for StudioOfflineAlert.vue. Would it be alright if I add the unit tests in a follow-up PR?
There was a problem hiding this comment.
Thanks! Generally it's good to have a test together with the logic it tests in a single PR since it's related. If you don't have time now, that's fine. I wouldn't mind waiting a bit before we merge, or if it'd be a long time then yes, a follow-up pull request is an option too.
There was a problem hiding this comment.
Also note that for new tests, we'd want to use Vue Testing Library as it has several benefits. It is installed in Studio. We have a guide for it here in Kolibri documentation, but it applies to Studio testing as well. I forgot to mention it in the issue, will update.
There was a problem hiding this comment.
Sure, it won't take a long time - I will include the tests in this PR.
| } | ||
|
|
||
| .alert-content span { | ||
| vertical-align: bottom; |
There was a problem hiding this comment.
This style doesn't have any effect.
Can be removed.
There was a problem hiding this comment.
It was a mistake. I’ll remove it 👍
| :style="computedstyle" | ||
| data-test="studio-offline-alert" | ||
| > | ||
| <div class="alert-content"> |
There was a problem hiding this comment.
.studio-offline-alert is too flex and in styles I don't see anything that would require having this another flex wrapper .alert-content around KIcon and offline text. Can it be simplified and .alert-content div removed, or was there any need for it?
There was a problem hiding this comment.
Initially, I added it to maintain the layout structure, but it's not necessary since the parent container already handles the spacing well. Happy to remove it to keep things cleaner
contentcuration/contentcuration/frontend/shared/views/StudioOfflineAlert.vue
Outdated
Show resolved
Hide resolved
| data-test="studio-offline-alert" | ||
| > | ||
| <div class="alert-content"> | ||
| <KIcon |
There was a problem hiding this comment.
As I resize my browser screen to smaller, the icon gets smaller too. I would expect it to be the same size on all screens. I think it gets smaller because it's default flex behavior. Let's keep using flex, it's good solution for this task, I think you will just need to tweak it a bit, probably with flex-shrink?
There was a problem hiding this comment.
Thanks for the guide — I will make the changes👍
| @@ -0,0 +1,95 @@ | |||
| <template> | |||
|
|
|||
| <KTransition kind="component-vertical-slide-out-in"> | |||
There was a problem hiding this comment.
There's a little visual hiccup - when transition is disappearing, it gradually moves too high over the yellow navigation bar. We've recently encountered the same issue here when attempting to apply component-vertical-slide-out-in on opening / closing this form.
In this particular case it's not so apparent, so I don't think it's blocking this PR. However I think it should be relatively simple fix in KDS repo that would update KTransition to transition in a similar way that Vuetify does. Let me know if that's something you'd like to fine-tune, or if I should rather open a follow-up issue.
There was a problem hiding this comment.
I checked on the issue — it’s caused by this line in KTransition, where transform: translateY(-50%) shifts the element up by 50% of its height . Replacing it with a smaller fixed value like -10px gives a much smoother effect without overlapping. If that works, I'm happy to make a PR to KDS with fix 👍
There was a problem hiding this comment.
That makes sense. KDS fix PR would be much appreciated.
There was a problem hiding this comment.
Thanks, should I create an issue for this, or can I go ahead without one?
There was a problem hiding this comment.
Feel free to go ahead without creating an issue - just reference my handle and this conversation, so it doesn't come out out of nothing in case other team members would preview :)! Thanks a lot.
Co-authored-by: Michaela Robosova <[email protected]>
Summary
Migrated the OfflineText Vuetify component with a new StudioOfflineAlert component that uses Kolibri Design System by following the guidelines .
KDS components used:
Manual verification steps performed:
UI screenshots:
Desktop:
Mobile:
References
Reviewer guidance