Skip to content

Refactor CodePaths and FileCodeSnippet components#1523

Merged
koesie10 merged 2 commits intomainfrom
koesie10/refactor-common-components
Sep 22, 2022
Merged

Refactor CodePaths and FileCodeSnippet components#1523
koesie10 merged 2 commits intomainfrom
koesie10/refactor-common-components

Conversation

@koesie10
Copy link
Copy Markdown
Member

This refactors the CodePaths and FileCodeSnippet components to be more readable and in style with the rest of the "new" components. It does the following:

  • Remove uses of the style and sx props; replace it by using styled-components instead
  • Remove uses of Primer icons
  • Split out the components into multiple files
  • Change the colors of the severity to match VSCode colors (and make them themable)

I haven't removed the use of the Primer Overlay component yet, since this component seems to do quite a lot and the VSCode WebView UI Toolkit doesn't have a replacement for it.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested review from a team as code owners September 20, 2022 09:48
@charisk
Copy link
Copy Markdown
Contributor

charisk commented Sep 20, 2022

I imagine it's a bit too late but would it be possible to have two separate commits/PRs to show off the refactoring and the moving separately? It's quite hard to know what has changed from the existing components just by looking at the diff. Feel free to say it'd be too much effort to do that now, and I'll take the hit with reviewing time.

@koesie10 koesie10 force-pushed the koesie10/refactor-common-components branch from 9bc1c76 to 8797432 Compare September 20, 2022 11:01
@koesie10
Copy link
Copy Markdown
Member Author

I imagine it's a bit too late but would it be possible to have two separate commits/PRs to show off the refactoring and the moving separately? It's quite hard to know what has changed from the existing components just by looking at the diff. Feel free to say it'd be too much effort to do that now, and I'll take the hit with reviewing time.

I've split it out into two commits: one for moving the files to the correct location and one for the refactoring!

@charisk
Copy link
Copy Markdown
Contributor

charisk commented Sep 20, 2022

I imagine it's a bit too late but would it be possible to have two separate commits/PRs to show off the refactoring and the moving separately? It's quite hard to know what has changed from the existing components just by looking at the diff. Feel free to say it'd be too much effort to do that now, and I'll take the hit with reviewing time.

I've split it out into two commits: one for moving the files to the correct location and one for the refactoring!

Sorry I think I did a terrible job at explaining things! Let me try again :) At the moment, the PR is a bit hard to review as changes to existing code since there are no separate commits for various bits of refactoring you've made. From the diff it looks like a bunch of deletions and a bunch of additions. It makes it quite hard to spot actual differences between the original code and the new code. Does that make sense?

Unrelated to the above, I also want to call out that I don't think we necessarily need to have each component live in a separate file. If a component is built up from a bunch of components, but those components aren't necessarily re-usable, then they can stay within the same file of the "parent" component. For example, the CodeFlowsDropdown is only something you can use within the CodePaths component. I don't object to breaking things out and having an index.ts that makes it clear what the interface is, I just wanted to mention that I think it should be a hard rule.

@koesie10
Copy link
Copy Markdown
Member Author

Sorry I think I did a terrible job at explaining things! Let me try again :) At the moment, the PR is a bit hard to review as changes to existing code since there are no separate commits for various bits of refactoring you've made. From the diff it looks like a bunch of deletions and a bunch of additions. It makes it quite hard to spot actual differences between the original code and the new code. Does that make sense?

I think it made sense what you said and completely agree that it just looked like some files were deleted and added. However, you should now be able to look at the two commits separately and the diffs should make sense: 8985ffe shows moving the files to the correct location and 8797432 shows the refactoring of the component itself (and the diff shows the changes within the file). The diff is still quite large, but I'm not sure whether creating more commits for it would make it easier to review.

Unrelated to the above, I also want to call out that I don't think we necessarily need to have each component live in a separate file. If a component is built up from a bunch of components, but those components aren't necessarily re-usable, then they can stay within the same file of the "parent" component. For example, the CodeFlowsDropdown is only something you can use within the CodePaths component. I don't object to breaking things out and having an index.ts that makes it clear what the interface is, I just wanted to mention that I think it should be a hard rule.

👍, I agree that we don't need to split every component. I think it's easier to follow if everything is in a separate file, but I'm also happy with keeping smaller components inside 1 file.

This refactors the CodePaths and FileCodeSnippet components to be more
readable and in style with the rest of the "new" components. It does the
following:

- Remove uses of the `style` and `sx` props; replace it by using
  `styled-components` instead
- Remove uses of Primer icons
- Split out the components into multiple files
- Change the colors of the severity to match VSCode colors (and make
  them themable)

I haven't removed the use of the Primer `Overlay` component yet, since
this component seems to do quite a lot and the VSCode WebView UI Toolkit
doesn't have a replacement for it.
@koesie10 koesie10 force-pushed the koesie10/refactor-common-components branch from 8797432 to 3817133 Compare September 21, 2022 09:31
@charisk
Copy link
Copy Markdown
Contributor

charisk commented Sep 22, 2022

I haven't removed the use of the Primer Overlay component yet, since this component seems to do quite a lot and the VSCode WebView UI Toolkit doesn't have a replacement for it.

Note that Design would like to revisit this area at some point so I expect the Primer Overlay to go away naturally then.

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for tidying up!

@koesie10 koesie10 enabled auto-merge September 22, 2022 08:52
@koesie10 koesie10 merged commit 9c07615 into main Sep 22, 2022
@koesie10 koesie10 deleted the koesie10/refactor-common-components branch September 22, 2022 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants