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

Add ability for method modeling panel to render multiple modelings of the same method #2921

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

koesie10
Copy link
Member

@koesie10 koesie10 commented Oct 6, 2023

This makes it possible for the method modeling panel to render multiple models for the same method. It does not yet add support for updating (i.e. onChange is unchanged) or adding/removing new methods. The appearance and behavior when the feature flag is disabled should be unchanged.

The best way to view this locally is using Storybook; there are 3 different scenarios in "Method Modeling/Method Modeling".

Screenshot 2023-10-06 at 16 52 03

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 October 9, 2023 07:29
@koesie10 koesie10 requested review from a team as code owners October 9, 2023 07:29
Copy link
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

}
`;

export const CodiconButton = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Is <VSCodeButton appearance="icon"><Codicon .../></VSCodeButton> not able to do what we want here? What are the challenges that require a new button component?

I'm not against adding a new component, but I just want to verify that it is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I didn't know that this prop existed. I'll switch to use this instead. Unfortunately it makes testing a bit harder (because they're not native components), but it does mean we'll have less custom components to maintain.

@koesie10 koesie10 force-pushed the koesie10/modeling-panel-multiple-models branch from 02e6e9f to 323c536 Compare October 9, 2023 11:52
@koesie10 koesie10 merged commit 5c368ce into main Oct 9, 2023
14 checks passed
@koesie10 koesie10 deleted the koesie10/modeling-panel-multiple-models branch October 9, 2023 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants