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

Refactor icons to not use React.FC #499

Open
2 tasks
nebula-aac opened this issue Feb 9, 2024 · 2 comments · May be fixed by #592
Open
2 tasks

Refactor icons to not use React.FC #499

nebula-aac opened this issue Feb 9, 2024 · 2 comments · May be fixed by #592
Assignees
Labels
kind/enhancement Improvement in current feature v0.16.0-planning

Comments

@nebula-aac
Copy link
Contributor

nebula-aac commented Feb 9, 2024

See previous open issue here: #224

Needed to open another ticket to track towards making this change for v0.16.0.

React has changes it type definitions which means React.FC will no longer be available to use.

  • Update each of the icons to use the appropriate props
    - [ ] Consider using SvgIcon from @mui/material as it has other properties we can benefit from using
  • Widen the IconProps to accept sx props from MUI in order to utilize the theme object inline

Contributor Guide

@nebula-aac nebula-aac added kind/enhancement Improvement in current feature v0.16.0-planning labels Feb 9, 2024
@nebula-aac nebula-aac changed the title Refactor icons to not useReact.FC Refactor icons to not use React.FC Feb 9, 2024
@senthil-athiban
Copy link
Contributor

@nebula-aac Should I refactor all the SVG icons to use the SvgIcon component from mui?

import { SVGProps } from 'react';
import { CARIBBEAN_GREEN_FILL, DEFAULT_HEIGHT, DEFAULT_WIDTH } from '../../constants/constants';
import { SvgIcon } from '@mui/material';

const ColumnIcon = ({
  width = DEFAULT_WIDTH,
  height = DEFAULT_HEIGHT,
  fill = CARIBBEAN_GREEN_FILL,
  ...props
}: SVGProps<SVGSVGElement>) => {
  return (
    <SvgIcon>
      <svg
        width={width}
        height={height}
        xmlns="http://www.w3.org/2000/svg"
        viewBox="0 -960 960 960"
        fill={fill}
        {...props}
      >
        <path d="M120-200v-560h220v560H120Zm250 0v-560h220v560H370Zm250 0v-560h220v560H620Z" />
      </svg>
    </SvgIcon>
  );
};

export default ColumnIcon;

@nebula-aac
Copy link
Contributor Author

@senthil-k8s Hello thank you for your question

Initially I considered doing this, because when I was testing it in the open PR I had for testing new changes to move to Next.js 14, there are underlying issues with using the MUI svg api to replace ours, which can't be done all at once, but rather needs to be one by one. So this issue needs to be changed to reflect the possible blockers.

@nebula-aac nebula-aac self-assigned this Apr 26, 2024
@nebula-aac nebula-aac linked a pull request Apr 27, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvement in current feature v0.16.0-planning
Development

Successfully merging a pull request may close this issue.

2 participants