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

Migrate TechRadar components to React Hooks + TypeScript #661

Closed
bih opened this issue Apr 27, 2020 · 17 comments
Closed

Migrate TechRadar components to React Hooks + TypeScript #661

bih opened this issue Apr 27, 2020 · 17 comments
Assignees
Labels
good first issue Good for newcomers help wanted Help/Contributions wanted from community members plugin A plugin is an extension of Backstage

Comments

@bih
Copy link
Contributor

bih commented Apr 27, 2020

Screenshot 2020-04-24 at 14 44 51

Related to the https://github.com/spotify/backstage/pull/628/files PR.

Feature Suggestion

For the plugins/tech-radar plugin for the Tech Radar we migrated the components which were internally written using class based components, higher order components (specifically withStyles from @material-ui/core) and without TypeScript.

In order for them to be in par with the rest of Backstage, they need updating:

  • Use makeStyles instead of withStyles from @material-ui/core
  • Use TypeScript
  • Use React functional components
  • Stretch: Add some additional unit tests for components

The components in question are located in plugins/tech-radar/src/components and here is a list:

Possible Implementation

See above.

Context

Ensure the codebase is using 100% React Hooks, TypeScript and modern practices.

@bih bih added help wanted Help/Contributions wanted from community members good first issue Good for newcomers plugin A plugin is an extension of Backstage labels Apr 27, 2020
@vedantrathore
Copy link

@bih I would like to work on this. Just found out about backstage, although I've currently not laid my hands on Typescript but I do have some experience with React so contributing to a production ready project would help me learn it. If you don't mind.

@bih
Copy link
Contributor Author

bih commented Apr 27, 2020

@vedantrathore Assigned! Let us know if you get stuck, more than happy to review :)

@bih
Copy link
Contributor Author

bih commented Apr 29, 2020

@vedantrathore How is it going?

@vedantrathore
Copy link

Hey @bih, I've setup the backstage local development environment and I'm currently going through the source code and tweaking things here and there to understand how everything works and how Typescript is used in the project. I'm also reading more about Typescript because I don't have any experience with it.

I'll draft out a PR for the Radar component converted into Typescript till this weekend (sorry, couldn't do it faster as I have a full-time job as well 😞) and you can validate and correct me if I'm going in the wrong direction.

Also I couldn't join the Discord channel tho, It was saying that the invite was invalid. Can you look into that once please?

Thanks for help!

@bih
Copy link
Contributor Author

bih commented Apr 29, 2020

No worries! Was just curious to see if there’s anything I can do to help. Can review over the weekend too. I’ll look into the Discord invite URL!

@vedantrathore
Copy link

vedantrathore commented May 2, 2020

Hey @bih, I read up about typescript and really love the philosophy behind using a strongly typed language. I do have a couple of noob doubts while I was working on refactoring the Radar component into Typescript though. It uses these proptypes which can be converted into the following types in typescript and can be passed into the props.

Radar.propTypes = {
  width: PropTypes.number.isRequired, // Required<number>;
  height: PropTypes.number.isRequired, // Required<number>;
  quadrants: PropTypes.arrayOf(PropTypes.object).isRequired, // Required<RadarQuadrant[]>;
  rings: PropTypes.arrayOf(PropTypes.object).isRequired, // Required<RadarRing[]>;
  entries: PropTypes.arrayOf(PropTypes.object).isRequired, // Required<RadarEntry[]>;
  svgProps: PropTypes.object, // object
};

So in the first static function adjustQuadrants it takes input the following input: adjustQuadrants(quadrants, radius, width, height) and eventually iterates over the quadrants and does some calculation on each quadrant using the following code.

quadrants.forEach((quadrant, idx) => {
  const legendParam = legendParams[idx % 4];

  quadrant.idx = idx;
  quadrant.radialMin = (idx * Math.PI) / 2;
  quadrant.radialMax = ((idx + 1) * Math.PI) / 2;
  quadrant.offsetX = idx % 4 === 0 || idx % 4 === 3 ? 1 : -1;
  quadrant.offsetY = idx % 4 === 0 || idx % 4 === 1 ? 1 : -1;
  quadrant.legendX = legendParam.x;
  quadrant.legendY = legendParam.y;
  quadrant.legendWidth = legendParam.width;
  quadrant.legendHeight = legendParam.height;
});

now since each quadrant object is of the type RadarQuadrant so the variables such as idx, radialMin etc were not available in the type definitions in api.ts so I added them accordingly.

export interface RadarQuadrant {
  id: string;
  name: string;
  idx: number | undefined; // I used undefined because these values are only modified in the adjustQuadrants method and are not from the api output. 
  radialMin: number | undefined;
  radialMax: number | undefined;
  offsetX: number | undefined;
  offsetY: number | undefined;
  legendX: number | undefined;
  legendY: number | undefined;
  legendWidth: number | undefined;
  legendHeight: number | undefined;
}

A similar kinda process is being followed in the next method adjustRings as well where it modifies the RadarRing type. So I'm unsure whether this is the best way to proceed. Should I modify the existing types with optional params or make the interface Partial or extend this interface altogether to use in the Component?

EDIT:

As @freben pointed out we can use the optional fields in interface definition but still is this the correct way to move forward?

export interface RadarQuadrant {
  id: string;
  name: string;
  idx?: number; // I used undefined because these values are only modified in the adjustQuadrants method and are not from the api output. 
  radialMin?: number;
  radialMax?: number;
  offsetX?: number;
  offsetY?: number;
  legendX?: number;
  legendY?: number;
  legendWidth?: number;
  legendHeight?: number;
}

@vedantrathore
Copy link

vedantrathore commented May 3, 2020 via email

@freben
Copy link
Member

freben commented May 3, 2020

Let's see what @bih says, but my two cents:

What's happening here is we're seeing an unfortunate effect of reusing DTOs for runtime dynamic purposes. Idealistically i would have wanted the RadarQuadrant split apart in one type for its rendering, and one for its metadata - what comes back from a service should strictly be immutable. But what you are doing sounds like it makes sense given the circumstances. If it is to be refactored, that can be done separately some other day; what we have works, after all.

@bih
Copy link
Contributor Author

bih commented May 4, 2020

Hello @vedantrathore! Yep using the PropTypes is a great way to figure out the types you'll want to use. +1 on @freben's comment too.

Most of this logic is quite old and created several years ago, with some unusual React patterns, so the logic itself does need to be rethought. For your peace of mind, I do think it's out of scope for this issue though. Of course, if you'd like to, that's fine too - but ideally in a separate PR/issue than the one for this issue.

A similar kinda process is being followed in the next method adjustRings as well where it modifies the RadarRing type. So I'm unsure whether this is the best way to proceed. Should I modify the existing types with optional params or make the interface Partial or extend this interface altogether to use in the Component?

I would just have a separate interface that extends from it. Interfaces in the api.ts should be what anyone using the API should know about. I don't think they should know about idx, radialMin, etc. Something like:

interface CalculatedRadarQuadrant extends RadarQuadrant {
  idx?: number; // I used undefined because these values are only modified in the adjustQuadrants method and are not from the api output. 
  radialMin?: number;
  radialMax?: number;
  offsetX?: number;
  offsetY?: number;
  legendX?: number;
  legendY?: number;
  legendWidth?: number;
  legendHeight?: number;
}

And this doesn't need to belong in the api.ts as it's an internal implementation detail. This can just go inside the .ts file where it's being used.

Hope this helps!

@bih
Copy link
Contributor Author

bih commented May 12, 2020

@vedantrathore Any updates on this from your side?

@vedantrathore
Copy link

@bih Apologies for the delay. While commenting on this issue in the beginning, I felt that I would be able to manage my time and work on this as well but the past few weeks have been super busy and unfortunately I couldn't. I'll unassign myself from this issue and hope to contribute in backstage in the future. Apologies for the inconvenience caused.

@vedantrathore vedantrathore removed their assignment May 14, 2020
@bih
Copy link
Contributor Author

bih commented May 14, 2020

@vedantrathore No worries at all, thanks for showing interest in volunteering and hopefully we'll see you get involved in the near future ;)

@NWCalvank
Copy link
Contributor

Hi, I should have time to pick up this refactor if it's still looking for an owner 🙂

@bih
Copy link
Contributor Author

bih commented May 21, 2020

@NWCalvank Absolutely! It's assigned to you :) Let us know if you need any guidance!

@german-j-rodriguez
Copy link
Contributor

Not sure if this is really a good place for the comment, but have you looked into https://github.com/AOEpeople/aoe_technology_radar seems like good piece to integrate

@NWCalvank
Copy link
Contributor

Hey everybody 👋

My PR was just merged: #1080 (comment)

I noted some further work that could be done, though I think the issue as-described here has been fully satisfied.

If you'd prefer to keep the issue open and refine the requirements for closing it, it's probably best to unassign me for the time being, as I likely won't be committing development time here for at least a week or two 🙂

Thanks! It was great to contribute again

@stefanalund
Copy link
Contributor

@NWCalvank thanks again for your contributions! If you don't mind, please open issues for the remaining tasks as you see them 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Help/Contributions wanted from community members plugin A plugin is an extension of Backstage
Projects
None yet
Development

No branches or pull requests

6 participants