Skip to content

Replace Material Icons with Kolibri Design System Icon #4913

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

Merged
merged 3 commits into from
Mar 21, 2025

Conversation

SukhvirKooner
Copy link
Contributor

Summary

Fixes: #4907
This PR removes all instances of material-icons from the codebase and replaces them with corresponding icons from the Kolibri Design System where applicable. In cases where no direct replacement was available, the icons were removed (e.g., in base.html).

Changes Made

  • Replaced all usages of material-icons with equivalent Kolibri Design System icons.
<svg
      v-else
      viewBox="0 0 40 40"
      :aria-label="kindTitle"
      class="nothumbnail-image"
      :class="$isRTL ? 'rtl-image' : 'ltr-image'"
    >
    <KIcon icon="image" 
        :x="-3"
        :y="y - 14"
        :style="{ fill: '#999999' }"  />
    </svg>

Before

Screenshot 2025-02-14 at 11 56 37 PM

After

Screenshot 2025-02-14 at 11 59 13 PM

<svg
      v-else-if="compact"
      viewBox="0 0 24 24"
      :aria-label="kindTitle"
      class="thumbnail-image"
    >
    <KIcon icon="infoOutline" 
        :x="+10"
        :y="y+20"
        :style="{ fill: '#ffffff' }"  />
    </svg>

Before

Screenshot 2025-02-14 at 11 58 18 PM

After

Screenshot 2025-02-14 at 11 59 37 PM

<svg
      v-else
      viewBox="0 0 40 40"
      :aria-label="title"
      class="nothumbnail-image"
      :class="$isRTL ? 'rtl-image' : 'ltr-image'"
    >
    <KIcon icon="image" 
        :x="-1"
        :y="y - 14"
        :style="{ fill: '#999999' }"  />
    </svg>

Before

Screenshot 2025-02-15 at 12 10 53 AM

After

Screenshot 2025-02-15 at 12 12 50 AM

  • Removed instances where material-icons were referenced but not actively used. in base.html

Testing

  • Verified that all updated icons render correctly in the UI.

  • Ensured no broken references remain in the code.

  • Manually tested affected pages to confirm visual correctness.

  • Please feel free to share any suggestions or further changes you'd like me to make. I'm happy to update the PR as needed. Looking forward to your feedback!

@@ -26,19 +26,15 @@
</div>
<svg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No update needed yet, just to understand the need to keep svg - if we removed svg and kept only KIcon, would there be a difference in user experience and what it would look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

  • Removing the <svg> makes the icon appear like in the screenshot, where it loses proper responsiveness and alignment.
  • Burying the icon within an <svg> ensures better adaptability since font-size scaling is harder to manage consistently.
  • Using <svg> provides better control over size, alignment, and responsiveness across different screen sizes and layouts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @SukhvirKooner, that's helpful. The proper alignment should be achievable no matter of the presence of svg. However KIcon doesn't have auto-scaling feature as of now so it's fine to keep it withing svg to not have regressions.

We may want to consider having these behaviors supported by KIcon itself at some point in the future.

@MisRob
Copy link
Member

MisRob commented Feb 17, 2025

Thanks @SukhvirKooner, and as always, I appreciate that you manually tested thoroughly and provided us with such a helpful pull request description. For now just leaving a few clarification questions.

@rtibbles rtibbles self-assigned this Feb 19, 2025
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good in manual testing as well.

If you're able to make two small updates, that would be great.

First we can delete the import of the material css: https://github.com/learningequality/studio/blob/unstable/contentcuration/contentcuration/frontend/shared/styles/main.scss#L1

And then remove the package entirely by doing yarn remove material-icons and then committing the changes to the package.json and yarn.lock file.

@SukhvirKooner
Copy link
Contributor Author

SukhvirKooner commented Mar 3, 2025

Hi @rtibbles,
After removing the import statement, some icons got unpublished. Since these icons were used in different ways instead of material-icons class, I'll fix them and update the PR shortly. Thanks for the review!
Here are few examples below:

Screenshot 2025-03-03 at 8 29 55 PM Screenshot 2025-03-03 at 8 38 32 PM

@rtibbles
Copy link
Member

rtibbles commented Mar 3, 2025

Ah - glad you were able to spot this before we pushed on! Thanks for being diligent about this. If you can spot any other ways these icons are being used, that would be very helpful!

@SukhvirKooner
Copy link
Contributor Author

Hi @rtibbles ,
I was engaged with my examinations over the past week, and now that they’re complete, I’ll be resuming work on this. Looking forward to continuing from here.

@MisRob
Copy link
Member

MisRob commented Mar 11, 2025

Thanks @SukhvirKooner. Take the time you need. I hope exams went well.

@rtibbles
Copy link
Member

Thanks @SukhvirKooner - very happy to hear from you, I look forward to supporting you to get this wrapped up!

@SukhvirKooner
Copy link
Contributor Author

Hi @rtibbles ,
I have found that Material Icons are being used indirectly in the code through a configuration and mapping system.
Here's how it works:

  1. Text as Icon Identifier:
    When we use <VIconWrapper>, it passes a text string like "error", "check_circle", or a dynamic expression such as {{ node.resource_count ? "folder" : "folder_open" }}. This text is interpreted as the icon name.

  2. Mapping System:
    The project defines mapping objects (e.g., ICONS_MATERIAL) that associate these icon names with specific Material Icon glyphs. For example, when you pass "error", the component looks up the mapping (defaulting to 'md' for Material Icons) and finds the corresponding Material Icon.

  3. Default Icon Set Configuration:
    The system is configured to use the 'md' (Material Design) icon set by default. This means that when <VIconWrapper> receives an icon name, it looks it up in the Material Icons mapping and applies the corresponding Material Icon—even if you never explicitly add a CSS class like "material-icons" in your templates.

  4. Component Abstraction:
    The custom <VIconWrapper> component acts as a layer between your application code and the underlying Vuetify icon system. It retrieves the icon name, consults the mapping, and renders the actual icon by applying the correct font-family or CSS classes behind the scenes.

In summary, even though the templates only pass simple text (the icon names), the mapping and default configuration cause these to be rendered as Material Icons.


I will continue work on this. If you have any suggestions on how to proceed with switching to use KIcons instead of Material Icons, please let me know.

@MisRob
Copy link
Member

MisRob commented Mar 14, 2025

@SukhvirKooner I don't think that VIconWrapper is in the scope of #4907, or @rtibbles did you mean it that way?

For context, we started that migration in #4502 and then I attempted to finish it in #4633 but the latter revealed many other dependencies that will need to be taken care of - it's very big scope. I plan to separate and spec this work to smaller chunks as I'm planning Studio to KDS refactor - before that I am pretty sure we can't do this. There are some performance concerns around tooltips, and other things to take care of first before a new attempt is possible.

@SukhvirKooner
Copy link
Contributor Author

Hi @MisRob ,
If that’s the case, then removing the import statement
@import '~material-icons/iconfont/material-icons.css;
wouldn’t be possible right now, since VIconWrapper still relies on the Material Icons font and CSS to render the icons correctly. Until VIconWrapper is migrated or refactored to use KIcon (or another solution), removing the material icons CSS would break those icons.
The current changes I’ve made should help move the migration forward, making the transition away from Material Icons easier.

@rtibbles
Copy link
Member

Ah, I hadn't anticipated this intersection. I think with this information @SukhvirKooner we can instead ignore my previous suggestion to completely remove the material icons dependency.

As @MisRob plans out the rest of the work you will be welcome to pick up other issues. Let's merge this as is rather than get more into the weeds!

@SukhvirKooner
Copy link
Contributor Author

Sounds good, thanks for the clarification! I'll leave it with you to merge.

@MisRob MisRob requested a review from rtibbles March 19, 2025 04:42
@MisRob
Copy link
Member

MisRob commented Mar 19, 2025

Sounds good to me. I think the diff makes sense, but will let @rtibbles to click the button since he reviewed. Thanks both!

@@ -553,10 +553,6 @@

.printing /deep/ * {
font-family: 'Noto Sans', helvetica !important;

&.material-icons {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, because of the continued use of VIconWrapper in Thumbnail.vue, this is still needed. The alternative would be much closer to the full swap out of Vicon for KIcon we agreed to avoid for now, so let's just revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No Problem! I just added it back.

@MisRob MisRob requested a review from rtibbles March 20, 2025 15:26
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good, no issues in manual testing. Thank you @SukhvirKooner!

@rtibbles rtibbles merged commit b281742 into learningequality:unstable Mar 21, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove use of material-icons library from code base
3 participants