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-5787 - Refactor EntityMetadata to simplify experimental/non-experimental logic and pull logic out of view #5315

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

nickgros
Copy link
Contributor

@nickgros nickgros commented Mar 6, 2024

No description provided.

@@ -19,23 +19,23 @@ public interface BooleanCallback {
void run(boolean value);
}

String entityId;
boolean show;
public String entityId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exposed to validate in test


@JsNullable
Double versionNumber;
public Long versionNumber;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why this was ever Double. Long works and is more correct.

Copy link
Member

Choose a reason for hiding this comment

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

Long has required special consideration in GWT. Maybe the dev read this in the compatibility notes: "Additionally, long primitives cannot be used in JSNI code because they are not a native JavaScript numeric type."

private EntityActionMenu actionMenu;
private final EntityModalWidget entityModalWidget;

private boolean annotationsAreVisible = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Store visibility state in the widget/presenter instead of the view

@@ -58,7 +61,8 @@ public EntityMetadata(
SynapseJSNIUtils jsni,
RestrictionWidget restrictionWidgetV2,
ContainerItemCountWidget containerItemCountWidget,
PortalGinInjector ginInjector
PortalGinInjector ginInjector,
EntityModalWidget entityModalWidget
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of this widget and its view directly rendering the React component, refactor so that the React component is wrapped in and abstracted by this EntityModalWidget.

Comment on lines +195 to 196
@Override
public void setAnnotationsVisible(boolean visible) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any change to annotation visibility should go through presenter, since it needs to store current visibility in a local instance variable

Comment on lines 198 to +203
if (DisplayUtils.isInTestWebsite(ginInjector.getCookieProvider())) {
view.setAnnotationsModalVisible(visible);
entityModalWidget.setOpen(visible);
view.setAnnotationsVisible(false);
} else {
view.setAnnotationsVisible(visible);
entityModalWidget.setOpen(false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to showing the correct annotation viewer based on experimental mode, hide the other one

}

public void setPresenter(Presenter presenter);

public void setDetailedMetadataVisible(boolean visible);

void setAnnotationsModalVisible(boolean visible);

boolean getAnnotationsVisible();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

View should not store state

}

public void setPresenter(Presenter presenter);

public void setDetailedMetadataVisible(boolean visible);

void setAnnotationsModalVisible(boolean visible);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modal visibility state now managed by EntityModalWidget

@nickgros nickgros merged commit a8b91d4 into Sage-Bionetworks:develop Mar 6, 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