Skip to content

Add call classification and supported kind to data extensions editor#2563

Merged
koesie10 merged 8 commits intomainfrom
koesie10/classification
Jul 14, 2023
Merged

Add call classification and supported kind to data extensions editor#2563
koesie10 merged 8 commits intomainfrom
koesie10/classification

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Jun 28, 2023

This will add a "Test" or "Generated" tag on the method if it's only used in tests or generated code and not in the actual application source code. It also adds retrieving the method "type" (e.g. source or sink) to the queries, but does not show these yet.

Screenshot 2023-07-13 at 16 46 04

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 marked this pull request as ready for review June 28, 2023 13:33
@koesie10 koesie10 requested a review from a team as a code owner June 28, 2023 13:33
@koesie10 koesie10 requested a review from starcke June 28, 2023 13:33
@koesie10 koesie10 force-pushed the koesie10/classification branch from ca86840 to 6a0cae5 Compare July 13, 2023 14:45
@koesie10 koesie10 requested a review from robertbrignull July 13, 2023 15:17
Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull left a comment

Choose a reason for hiding this comment

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

No comments on how the code is written. That all looks great.

My only comment on the styling is whether the classification labels are a bit too large and prominent, and if it makes the view more cluttered. It's the default styling so it's definitely not your fault, but they maybe stand out slightly too much and distract from other elements, and it's unhelpful that they look almost exactly the same as the clickable buttons.

However, I'd be happy to proceed as this is now, with the understanding that we'll be doing a design review once we get to the end of this current implementation work.

<ViewLink onClick={jumpToUsage}>View</ViewLink>
{!inSource && (
<ClassificationsContainer>
{inTest && <VSCodeTag>Test</VSCodeTag>}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Random thought. Is it possible to put a tooltip / title text on these that explains "This method is only used from test code"?

@koesie10
Copy link
Copy Markdown
Member Author

My only comment on the styling is whether the classification labels are a bit too large and prominent, and if it makes the view more cluttered. It's the default styling so it's definitely not your fault, but they maybe stand out slightly too much and distract from other elements, and it's unhelpful that they look almost exactly the same as the clickable buttons.

I agree, the tags are a little bit big. I've made their text sizes smaller so they stand out less, so this should hopefully partly fix that. I've also added the tooltip you suggested. Unfortunately, the webview UI toolkit doesn't have a tooltip component, so it's just a title on a <div> which shows when you hover over the tags for some time.

Copy link
Copy Markdown
Contributor

@robertbrignull robertbrignull 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 those styling tweaks, and putting the classification code in a new component looks good.

@koesie10 koesie10 merged commit 10d9213 into main Jul 14, 2023
@koesie10 koesie10 deleted the koesie10/classification branch July 14, 2023 10:02
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