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

SWC-6668: implement react-component for editing the definingSql #5295

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

jinjunoh
Copy link
Collaborator

…table instead of GWT

@jinjunoh jinjunoh requested a review from nickgros February 16, 2024 21:45
@@ -405,6 +407,7 @@ public void before() {
PromptForValuesModalView.Configuration.Builder.class,
new SelfReturningAnswer()
);
// when(mockMaterializedView.getId()).thenReturn(entityId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// when(mockMaterializedView.getId()).thenReturn(entityId);

@JsType(isNative = true, namespace = JsPackage.GLOBAL, name = "Object")
public class SqlDefinedTableEditorModalProps extends ReactComponentProps {

// going to match the props sent from src
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// going to match the props sent from src

@FunctionalInterface
@JsFunction
public interface OnUpdate {
void OnUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

camelCase for consistency. You could also just define one Callback interface for both onUpdate and onCancel, but that's up to you

Suggested change
void OnUpdate();
void onUpdate();

Copy link
Contributor

@nickgros nickgros left a comment

Choose a reason for hiding this comment

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

Looks good! We need to release a new version of SRC and pull it in before we merge this.

@nickgros nickgros merged commit 5c5458a into Sage-Bionetworks:develop Feb 20, 2024
2 checks passed
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