[DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS #4633

MisRob · 2024-08-11T06:41:53Z

Summary

Description of the change(s) you made

Almost completes icons migration from Vuetify to KDS.

Replaces almost all remaining VIconWrappers with KIcon, KLabeledIcon, or KIconButton
Contains related icon maps and utility function changes
Replaces VTooltip by KTooltip at places where KDS icons usage requires it
Removes global registrations of Vuetify icon
Contains few minor UI fixes
Removes unused code
Installs Let's teleport + make 'sidebar' icon flip in RTL languages kolibri-design-system#722 (to be replaced by the KDS release)
- To fix RTL flip for the sidebar icon
- To be able to teleport KTooltip to the teleport root element in few places where there seemed to be no other way to fix its parts being cut off (mostly related to being surrounded by Vuetify components)

See commit messages for details.

Note that the Thumbnail component still uses VIconWrapper. There are some features that we need to add first to KIcon to allow for the refactor. I will try to write what needs to happen to a KDS issue as soon as I have time for this agian. To prevent from using VIconWrapper except this one place, I moved it to the Thumbnail directory and left deprecation note.

Manual verification steps performed

I was gradually previewing all places where I worked.

Reviewer guidance

How can a reviewer test these changes?

Remove node_modules (otherwise you won't get updated KDS version in the installation step) and then run yarn install to ensure that Let's teleport + make 'sidebar' icon flip in RTL languages kolibri-design-system#722 work is available
Do code changes make sense?
Can you observe any icon related regressions? Note that you don't need to preview all places, as agreed with @bjester, there will be thorough pre-release QA.

References

Follow-up to @BabyElias's #4502

Contributor's Checklist

PR process:

~~If this is an important user-facing change, PR or related issue the CHANGELOG label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later time~~
~~If this includes an internal dependency change, a link to the diff is provided~~
~~The docs label has been added if this introduces a change that needs to be updated in the user docs?~~
~~If any Python requirements have changed, the updated requirements.txt files also included in this PR~~
~~Opportunities for using Google Analytics here are noted~~
~~Migrations are safe for a large db~~

Studio-specifc:

~~All user-facing strings are translated properly~~
~~The notranslate class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)~~
All UI components are LTR and RTL compliant
~~Views are organized into pages, components, and layouts directories as described in the docs~~
~~Users' storage used is recalculated properly on any changes to main tree files~~
~~If there new ways this uses user data that needs to be factored into our Privacy Policy, it has been noted.~~

Testing:

Code is clean and well-commented
Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
~~Critical user journeys are covered by Gherkin stories~~
~~Any new interactions have been added to the QA Sheet~~
~~Critical and brittle code paths are covered by unit tests~~

Reviewer's Checklist

This section is for reviewers to fill out.

Automated test coverage is satisfactory
PR is fully functional
PR has been tested for accessibility regressions
External dependency files were updated if necessary (yarn and pip)
Documentation is updated
Contributor is in AUTHORS.md

- Shows dropup icon when item is open - Makes dropdown/dropup icons size consistent with the channel dropdown/dropup

For slideshow and error, there's a slight visual difference, however it is aligned with the KDS. Includes a new icons map and renames the previous one to indicate that it is Vuetify specific and in the process of deprecation.

- prepares ground for adding related files - no changes to the file contents

Part of migration to KIcon. The icons removed are not currently used from anywhere in Studio. and if we need them in the future, they will be added to KDS.

to common utils file. It has nothing to do with Vuetify icons anymore.

This is part of migrating Studio to KIcon. Thumbnail.vue component is the only remaning component using VIconWrapper. It can't be migrated to KIcon yet since KIcon needs to be updated at first with some new features. Moving the files here ensures that VIconWrapper won't be used from anywhere else in the Studio.

Icon.vue was originally a temporary wrapper that offered some extra logic, such as RTL support. However, KIcon is taking care of it and the wrapper doesn't really do anything extra.

IconButton.vue was originally a temporary wrapper, oferring some extra logic such as RTL support. However, KDS takes care of RTL, so it's not needed. One more thing IconButton did was automatically setting aria label - if we're certain this is desired in all cases, it can be introduced as a KDS feature.

They need to be displayed next to the text rather to take the whole block.

MisRob · 2024-08-28T12:34:04Z

Resolved latest conflicts and updated to the latest changes in learningequality/kolibri-design-system#722

nucleogenesis

I added my code comment last week re: i18n, but failed to complete my "review" overall the changes look good to me.

I think it'd be worth getting some significant QA done on this for regressions in any case.

Should I do some manual QA on this? If so I can make time to do it tomorrow

nucleogenesis · 2024-08-12T23:39:05Z

contentcuration/contentcuration/frontend/administration/pages/Users/UserTable.vue

+            tooltip="Email"
+            ariaLabel="Email"


I was wondering if these should be translated but then I think I recall that we don't translate the administration app?

Good catch. Hmm I am not sure, I just used what was there. @bjester or @marcellamaki do you know?

Yeah generally we haven't worried about translating the administration side of Studio. So this is acceptable given the current state of it.

MisRob · 2024-08-29T19:23:28Z

Thank you @nucleogenesis

I think it'd be worth getting some significant QA done on this for regressions in any case.

Absolutely. I did my best to preview as I worked, but was returning to it repeatedly after longer periods of time, and now I also keep rebasing, so we will very likely find some trouble.

Should I do some manual QA on this? If so I can make time to do it tomorrow

That's a generous offer :) I think for developers it'd make more sense to manually preview a suspicious place in code. We talked with @bjester some time ago and agreed this would be rather task for the pre-release QA (which happens after merging to unstable if I understand correctly)

bjester · 2024-10-11T17:01:29Z

contentcuration/contentcuration/frontend/channelEdit/components/Clipboard/ContentNode.vue

@@ -62,7 +62,7 @@
                      <VListTileAction
                        class="action-col px-1 v-list__group__header__append-icon"
                      >
-                        <Icon icon="dropdown" />
+                        <KIcon :icon="open ? 'dropup' : 'dropdown'" :style="{ fontSize: '22px' }" />


Not a blocker, but I'm wondering if there's a way to make a nice visual transition between these two icons.

bjester

Everything looks pretty good. My main concern though, which I'm not sure whether it's a blocker yet, is whether KTooltip behaves similarly to VTooltip with the lazy prop. Previously, we found that because of the large number of tooltips, we can could cut down on the number of rendered elements and reduce memory usage slightly with the use of lazy. With these changes, if the differences aren't evaluated, we could end up increasing the memory usage of a channel page.

bjester · 2024-10-11T17:05:51Z

contentcuration/contentcuration/frontend/channelEdit/components/move/MoveModal.vue

                  </VBtn>
                  <VBtn
                    v-if="node.kind === 'topic'"
                    icon
-                    class="mx-1 rtl-flip"


Are we certain rtl-flip should be removed here?

bjester · 2024-10-11T17:11:00Z

contentcuration/contentcuration/frontend/shared/views/IconButton.vue

-    ariaLabel="text"
-    :color="color"
-    :size="size"
-    :class="{ 'rtl-flip': rtlFlip }"


If this component was essentially a wrapper for KIconButton, what were the reasons for having this apply the rtl-flip class? It seems that it would still be important unless this was because of a deficiency in the KDS component itself that has been fixed.

MisRob · 2024-10-15T15:53:09Z

Thanks @bjester, that's a good feedback. I will look into it all as soon as I'm back in November, since for some parts I think I will need more time.

Use KIcon in Administration

43f6bdb

MisRob force-pushed the complete-icons-migration branch from a4261a4 to 5bb1ce9 Compare August 11, 2024 06:58

MisRob added 28 commits August 11, 2024 09:01

Use KIcon in ContentNodeValidator

fdaeafb

Use KIcon in StudioTree

8480af3

Use KTooltip together with KIcon for coach content indicator

e52f6ac

Use KIcon in edit modal tabs

613599b

Use KIcon in LazyListGroup

0306be8

Icon fixes for clipboard sub-item

c35e9a8

- Shows dropup icon when item is open - Makes dropdown/dropup icons size consistent with the channel dropdown/dropup

Use KIcon in ContentNodeIcon

5556471

For slideshow and error, there's a slight visual difference, however it is aligned with the KDS. Includes a new icons map and renames the previous one to indicate that it is Vuetify specific and in the process of deprecation.

Fix missing space between icon and label

67c1e2b

Use KIcon in EditListItem

21b233e

Use KIcon in FileUpload

31a9155

Remove dead code

3312f98

Use KIcon in ImportFromChannelsModal

4401813

Use KIcon in ContentNodeCopyTaskProgress

da722f5

Use KIcon in the staging tree banner

63e429f

Use KIcon in ImmersiveModalLayout

23739a7

Use KIcon in FileStatus

fff37a2

Use KIcon in ContentRenderer

68cc4bc

Use KIcon in ChannelInfoCard

1475772

Move Thumbnail to a directory

755ab81

- prepares ground for adding related files - no changes to the file contents

Remove unused custom icons

71217e9

Part of migration to KIcon. The icons removed are not currently used from anywhere in Studio. and if we need them in the future, they will be added to KDS.

Move learning activity icon helper

a8a52e3

to common utils file. It has nothing to do with Vuetify icons anymore.

Remove global registrations of Vuetify icon

4718501

Remove obsolete test suite

926760e

Remove Icon wrapper

fb566c0

Icon.vue was originally a temporary wrapper that offered some extra logic, such as RTL support. However, KIcon is taking care of it and the wrapper doesn't really do anything extra.

Do not use hardcoded color

e38506f

Fix help tooltips position

652f59e

They need to be displayed next to the text rather to take the whole block.

MisRob force-pushed the complete-icons-migration branch 2 times, most recently from 54c4476 to cfa0e15 Compare August 11, 2024 08:53

Remove Teleport in favour of appendToRoot

9f9da8f

MisRob force-pushed the complete-icons-migration branch 3 times, most recently from 7640bc6 to 5494a6e Compare August 11, 2024 11:04

Add deprecation note

38cb5c8

MisRob force-pushed the complete-icons-migration branch from 5494a6e to 272be7e Compare August 11, 2024 13:07

MisRob mentioned this pull request Aug 11, 2024

Let's teleport + make 'sidebar' icon flip in RTL languages learningequality/kolibri-design-system#722

Merged

9 tasks

[TMP] Temporarily pin KDS to fork

14cfe2b

MisRob force-pushed the complete-icons-migration branch from 272be7e to 14cfe2b Compare August 11, 2024 13:30

MisRob changed the title ~~[WIP] Almost complete icons migration from Vuetify to KDS~~ [DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS Aug 11, 2024

MisRob requested review from bjester, nucleogenesis, AlexVelezLl and akolson August 11, 2024 14:06

MisRob added the TODO: needs review label Aug 11, 2024

MisRob marked this pull request as ready for review August 11, 2024 14:08

rtibbles assigned marcellamaki Aug 13, 2024

marcellamaki added this to the Studio: prioritization of releases/patches milestone Aug 27, 2024

MisRob force-pushed the complete-icons-migration branch from 94b2359 to 440f698 Compare August 28, 2024 11:48

Merge branch 'unstable' into complete-icons-migration

e0563de

MisRob force-pushed the complete-icons-migration branch from 440f698 to e0563de Compare August 28, 2024 12:14

[TMP] Update to the latest KDS

ff3c26c

nucleogenesis reviewed Aug 29, 2024

View reviewed changes

bjester reviewed Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS #4633

[DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS #4633

MisRob commented Aug 11, 2024 •

edited

Loading

MisRob commented Aug 28, 2024

nucleogenesis left a comment

nucleogenesis Aug 12, 2024

MisRob Aug 29, 2024

bjester Oct 11, 2024

MisRob commented Aug 29, 2024

bjester Oct 11, 2024

bjester left a comment

bjester Oct 11, 2024

bjester Oct 11, 2024

MisRob commented Oct 15, 2024

[DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS #4633

Are you sure you want to change the base?

[DO NOT MERGE UNTIL NEW KDS RELEASE] Almost complete icons migration from Vuetify to KDS #4633

Conversation

MisRob commented Aug 11, 2024 • edited Loading

Summary

Description of the change(s) you made

Manual verification steps performed

Reviewer guidance

How can a reviewer test these changes?

References

Contributor's Checklist

Reviewer's Checklist

This section is for reviewers to fill out.

MisRob commented Aug 28, 2024

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Aug 12, 2024

Choose a reason for hiding this comment

MisRob Aug 29, 2024

Choose a reason for hiding this comment

bjester Oct 11, 2024

Choose a reason for hiding this comment

MisRob commented Aug 29, 2024

bjester Oct 11, 2024

Choose a reason for hiding this comment

bjester left a comment

Choose a reason for hiding this comment

bjester Oct 11, 2024

Choose a reason for hiding this comment

bjester Oct 11, 2024

Choose a reason for hiding this comment

MisRob commented Oct 15, 2024

MisRob commented Aug 11, 2024 •

edited

Loading