komiser icon indicating copy to clipboard operation
komiser copied to clipboard

Tech improvement: Refactor direct embedding of SVGs

Open titanventura opened this issue 1 year ago • 7 comments

Is your feature request related to a problem? Please describe. Could be better if we can refactor the frontend components/pages to use SVGs from the public folder rather than hardcoding the SVGs as inline embeddings

Describe the solution you'd like Use next/image to serve the SVGs instead of using inlines. We could even use a library like svgr https://github.com/gregberge/svgr to do this. Would like to analyze.

Additional context Reference: https://github.com/tailwarden/komiser/blob/33567623e46ce27c7f8c9230c6d144139491466c/dashboard/components/dashboard/components/top-stats/DashboardTopStatsCards.tsx#L21

Am i willing to submit a PR ? YES

titanventura avatar Oct 09 '23 17:10 titanventura

@ShubhamPalriwala @Traxmaxx wdyt?

mlabouardy avatar Oct 09 '23 17:10 mlabouardy

Is your feature request related to a problem? Please describe. Could be better if we can refactor the frontend components/pages to use SVGs from the public folder rather than hardcoding the SVGs as inline embeddings

Describe the solution you'd like Use next/image to serve the SVGs instead of using inlines. We could even use a library like svgr https://github.com/gregberge/svgr to do this. Would like to analyze.

Additional context Reference:

https://github.com/tailwarden/komiser/blob/33567623e46ce27c7f8c9230c6d144139491466c/dashboard/components/dashboard/components/top-stats/DashboardTopStatsCards.tsx#L21

Am i willing to submit a PR ? YES

Hey transferring SVGs to the public folder and using Next/image to serve them might not be necessary because Next/image does not optimize SVGs. You can find more information about this in the Next.js documentation here: https://nextjs.org/docs/pages/api-reference/components/image#dangerouslyallowsvg

image

shobhitexe avatar Oct 10 '23 12:10 shobhitexe

the current way the SVGs are rendered are also not optimised, the next js documentation means compression as optimization.

titanventura avatar Oct 10 '23 12:10 titanventura

I just added #1080 shall I merge these two issues and update the desciption? It's good to get startet on it.

edit: I have no particular opinion if we want to load them via public paths or as react components as long as icons do not need to be animated at some point. In this case it's better to have them as react components. It also makes overriding styles with tailwind easier maybe 🤔

Traxmaxx avatar Oct 11 '23 14:10 Traxmaxx

Description from the other issue just for reference:

Is your feature request related to a problem? Please describe. We want to have an overview of existing Icons and which ones to add when working on new screens.

Describe the solution you'd like We should add all icons into a shared folder inder dashboard/components/icons and add all of them to storybook to make them searchable. Then replace the static ones with the new components.

Currently an static icon is inline svg. For example:

<svg
  width="24"
  height="24"
  viewBox="0 0 24 24"
  fill="none"
  xmlns="http://www.w3.org/2000/svg"
>
  <path
    d="M19.7897 14.9298C17.7297 16.9798 14.7797 17.6098 12.1897 16.7998L7.47966 21.4998C7.13966 21.8498 6.46966 22.0598 5.98966 21.9898L3.80966 21.6898C3.08966 21.5898 2.41966 20.9098 2.30966 20.1898L2.00966 18.0098C1.93966 17.5298 2.16966 16.8598 2.49966 16.5198L7.19966 11.8198C6.39966 9.21982 7.01966 6.26982 9.07966 4.21982C12.0297 1.26982 16.8197 1.26982 19.7797 4.21982C22.7397 7.16982 22.7397 11.9798 19.7897 14.9298Z"
    stroke="#697372"
    stroke-width="1.5"
    stroke-miterlimit="10"
    stroke-linecap="round"
    stroke-linejoin="round"
  />
  <path
    d="M6.88965 17.4902L9.18965 19.7902"
    stroke="#697372"
    stroke-width="1.5"
    stroke-miterlimit="10"
    stroke-linecap="round"
    stroke-linejoin="round"
  />
  <path
    d="M14.5 11C15.3284 11 16 10.3284 16 9.5C16 8.67157 15.3284 8 14.5 8C13.6716 8 13 8.67157 13 9.5C13 10.3284 13.6716 11 14.5 11Z"
    stroke="#697372"
    stroke-width="1.5"
    stroke-linecap="round"
    stroke-linejoin="round"
  />
</svg>

These static icons need to become a component. For example

import { SVGProps } from 'react';

const KeyIcon = (props: SVGProps<SVGSVGElement>) => (
  <svg
    width="24"
    height="24"
    viewBox="0 0 24 24"
    fill="none"
    xmlns="http://www.w3.org/2000/svg"
    {...props}
  >
    <path
      d="M19.7897 14.9298C17.7297 16.9798 14.7797 17.6098 12.1897 16.7998L7.47966 21.4998C7.13966 21.8498 6.46966 22.0598 5.98966 21.9898L3.80966 21.6898C3.08966 21.5898 2.41966 20.9098 2.30966 20.1898L2.00966 18.0098C1.93966 17.5298 2.16966 16.8598 2.49966 16.5198L7.19966 11.8198C6.39966 9.21982 7.01966 6.26982 9.07966 4.21982C12.0297 1.26982 16.8197 1.26982 19.7797 4.21982C22.7397 7.16982 22.7397 11.9798 19.7897 14.9298Z"
      stroke="#697372"
      stroke-width="1.5"
      stroke-miterlimit="10"
      stroke-linecap="round"
      stroke-linejoin="round"
    />
    <path
      d="M6.88965 17.4902L9.18965 19.7902"
      stroke="#697372"
      stroke-width="1.5"
      stroke-miterlimit="10"
      stroke-linecap="round"
      stroke-linejoin="round"
    />
    <path
      d="M14.5 11C15.3284 11 16 10.3284 16 9.5C16 8.67157 15.3284 8 14.5 8C13.6716 8 13 8.67157 13 9.5C13 10.3284 13.6716 11 14.5 11Z"
      stroke="#697372"
      stroke-width="1.5"
      stroke-linecap="round"
      stroke-linejoin="round"
    />
  </svg>
);

export default KeyIcon;

Beware of classes or stylings which need to be passed in as props when replacing the static icon!

Describe alternatives you've considered None

Traxmaxx avatar Oct 16 '23 09:10 Traxmaxx

please assign me

rafiya2003 avatar Oct 22 '23 09:10 rafiya2003

Sorry @rafiya2003 I just saw this now. All yours

jakepage91 avatar Dec 04 '23 14:12 jakepage91