Skip to content

Move components for rendering locations to a separate file#2614

Merged
robertbrignull merged 11 commits intomainfrom
robertbrignull/Locations
Jul 18, 2023
Merged

Move components for rendering locations to a separate file#2614
robertbrignull merged 11 commits intomainfrom
robertbrignull/Locations

Conversation

@robertbrignull
Copy link
Copy Markdown
Contributor

This PR achieves two goals:

  • Converts code from "functions that happen to return react components" like renderLocation to actual react components that can have things like state and hooks.
  • Pools together several similar and interdependent components for rendering locations.

This causes a slight increase in line count because we need to pass around more state as props rather than inheriting it from the environment. Arguably this is a benefit to long term readability and maintainability of the components by avoiding implicit coupling.

This PR is a potential stepping stone towards breaking up and modernising AlertTable. But this PR is not essential. I'm open to not grouping these functions into locations.tsx if this grouping of components doesn't make the code easier to understand.

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.

@robertbrignull robertbrignull requested a review from a team as a code owner July 17, 2023 13:48
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up! I believe most of my comments are about things that were already there in the old components, but can be improved as part of this cleanup as well.

Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations.tsx Outdated
Copy link
Copy Markdown
Member

@koesie10 koesie10 left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes!

Comment thread extensions/ql-vscode/src/view/results/locations/ClickableLocation.tsx Outdated
Comment thread extensions/ql-vscode/src/view/results/locations/ClickableLocation.tsx Outdated
@robertbrignull robertbrignull enabled auto-merge July 18, 2023 14:22
@robertbrignull robertbrignull merged commit fc77a52 into main Jul 18, 2023
@robertbrignull robertbrignull deleted the robertbrignull/Locations branch July 18, 2023 14:31
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