Skip to content
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

Tech improvement: Refactor direct embedding of SVGs #1058

Open
titanventura opened this issue Oct 9, 2023 · 7 comments
Open

Tech improvement: Refactor direct embedding of SVGs #1058

titanventura opened this issue Oct 9, 2023 · 7 comments

Comments

@titanventura
Copy link
Contributor

titanventura commented Oct 9, 2023

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:

Am i willing to submit a PR ?
YES

@titanventura titanventura added enhancement New feature or request feature labels Oct 9, 2023
@mlabouardy
Copy link
Collaborator

@ShubhamPalriwala @Traxmaxx wdyt?

@shobhitexe
Copy link
Contributor

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:

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

@titanventura
Copy link
Contributor Author

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

@Traxmaxx
Copy link
Contributor

Traxmaxx commented Oct 11, 2023

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
Copy link
Contributor

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

@rafiya2003
Copy link

please assign me

@jakepage91
Copy link
Contributor

Sorry @rafiya2003 I just saw this now. All yours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants